core(font-size): remove deprecated DOM.getFlattenedDocument #11248
Conversation
connorjclark
requested review from
patrickhulce
and removed request for
a team
| const nodes = [ | ||
| {nodeId: 1, backendNodeId: 100, nodeName: 'HTML'}, | ||
| {nodeId: 2, backendNodeId: 101, nodeName: 'HEAD', parentId: 1}, | ||
| {nodeId: 0, backendNodeId: 100, nodeName: 'HTML'}, |
There was a problem hiding this comment.
making these 0 index was easier to debug
| */ | ||
| calculateBackendIdsToFontData(snapshot) { | ||
| * iterateTextNodesInLayoutFromSnapshot(snapshot) { |
There was a problem hiding this comment.
I don't think we've ever used generator syntax in Lighthouse outside of the very unusual asset saver case which is very specific to allowing streamable results, probably deserves some sort of comment or justification :)
is there an actual benefit here compared to just doing a filtered map instead? AFAICT, we're just using the iterator to push all of these objects onto an array anyway, but maybe there were other memory pressure reasons I'm mising?
There was a problem hiding this comment.
outside of the very unusual asset saver case which is very specific to allowing streamable results
(and really is a product of the Node < 8.10 days when we couldn't have strings over 256MiB (#1685 (comment)) and no one has touched that save method for three years)
There was a problem hiding this comment.
Happy to change, not a sticking point, but I don't like the idea of avoiding entire language features because they are new or not commonly used :) Would a comment linking to "how generators work" help? Is the main concern just that someone doesn't know what * is, or that even if they do, do you think generators are hard to reason about?
I appreciate a generator here as the size of the data created is not bounded. It scales with the number of elements on the page, and with the amount of text within those elements. Since only a single value needs to be processed at a time, and because the creation of said value is rather complex, extracting as a generator function made sense to me.
The alternative seems to be:
- inline this entire function. affects readability, function decomposition is good, etc.
- return as an array. this is the current state, and I believe I reduced some of the concern here in a previous PR by returning the
textLengthinstead of thetext. Here I reverted that, as it's a derived property better calculated by the calling code.
There was a problem hiding this comment.
Is the main concern just that someone doesn't know what * is, or that even if they do, do you think generators are hard to reason about?
Mostly this. I wouldn't expect most developers to immediately understand how this function works compared to adding items to an array. For this reason, I would only really use a generator in a shared codebase when the benefits are significant (like in the string creation case of asset saver) which is accompanied by that justification in comments.
I believe I reduced some of the concern here in a previous PR by returning the textLength instead of the text. Here I reverted that, as it's a derived property better calculated by the calling code.
Ah, OK I didn't notice this. If we held on to text for any significant part of the filtering step I might agree with you this is sufficient justification, but much like your other simplifications in this PR, we could solve it by limiting the returned information and just change text to a textLength: getTextLength(text) invocation into the iterator function, right?
as it's a derived property better calculated by the calling code.
I'm not sure I follow this one, we'd still be calculating it with the exact same method now, we'd just compute it before returning the node object.
Final thoughts:
I understand your concern about refusing to adopt new language features and ending up with a ES20XX-era codebase forever. I'm not as worried about this being the case as we've extensively adopted several features newer to node than generators, it's primarily that generators are rarer in applicability for Lighthouse (and therefore in my belief less likely to be easy to read for the average developer) than asynchronous code.
While my primary concern was wrong to lean exclusively on "it's different, so no", there is still some merit to consistency in patterns. @paulirish mostly converted me here on avoiding reduce in pretty much every situation I would've used it previously for easier reading to those less accustomed to reduce as a general-purpose for-loop, and I believe in other recent reviews of new contributors we both concurred on use of a for...of instead of a long .filter().map().filter().forEach() chain. Not that there's never a time to use those new patterns, but for a situation in which the dominant pattern already fits well and the new pattern is sufficiently different in complexity of comprehension, I'd prefer the former.
| fontSize: number; | ||
| textLength: number; | ||
| node: FontSize.DomNodeWithParent; | ||
| parentNode: { |
There was a problem hiding this comment.
thanks for restructuring this, it makes much more sense this way 👍
* upstream/master: (42 commits) docs: add Code of Conduct to project (GoogleChrome#11212) docs(readme): add related project: lighthouse-viewer (GoogleChrome#11250) core(font-size): remove deprecated DOM.getFlattenedDocument (GoogleChrome#11248) misc: fix typo in method name (GoogleChrome#11239) i18n: make double dollar validation less strict (GoogleChrome#10299) misc: rephrase comments to be more inclusive (GoogleChrome#11228) misc: tweak gcp scripts to work in google corp (GoogleChrome#11233) v6.2.0 (GoogleChrome#11232) report: correctly display CLS in budget table (GoogleChrome#11209) report: vertically center thumbnails (GoogleChrome#11220) i18n: import (GoogleChrome#11225) tests: istanbul ignore inpage function (GoogleChrome#11229) deps(snyk): update script to prune <0.0.0 and update snapshot (GoogleChrome#11223) core(stacks): timeout stack detection (GoogleChrome#11172) core(config): unsized-images to default (GoogleChrome#11217) core(image-elements): collect CSS sizing, ShadowRoot, & position (GoogleChrome#11188) core: add FormElements gatherer (GoogleChrome#11062) new_audit: report animations not run on compositor (GoogleChrome#11105) tests: update chromestatus expecatations (GoogleChrome#11221) deps: update dot-prop secondary dependency (GoogleChrome#11198) ...
Fixes #11210 (went with option
resolve many at once using push node to frontend method)Reduced the scope of the FontSize artifact by exporting only what we need, not an entire CrdpNode.
Test url http://misc-hoten.surge.sh/lh-ui-location-font-size/ from #9354 shows that results are unchanged.