new_audit: add script-treemap-data to experimental#11271
Conversation
| /** | ||
| * @typedef Node | ||
| * @property {string} name | ||
| * @property {number} resourceBytes |
There was a problem hiding this comment.
might be convinced to use transferBytes instead ..
There was a problem hiding this comment.
resource seems fine for the treemap visualization (though eventually we might want both?), we don't exactly want to minimize the size of these over there :)
| const url = scriptElement.src; | ||
| const bundle = bundles.find(bundle => url === bundle.script.src); | ||
| const scriptCoverages = artifacts.JsUsage[url]; | ||
| if (!bundle || !scriptCoverages) continue; |
There was a problem hiding this comment.
when does this happen? If it happens for decent-sized scripts, should they be included in some way still? (even if just an "other" category or whatever). If it's just for type checking or shouldn't really occur, add a comment?
There was a problem hiding this comment.
This was accidentally skipping all scripts without a source map. Messed that up during some refactoring.
| expect(treemapData.scripts).toMatchSnapshot(); | ||
| }); | ||
|
|
||
| it('resource summary', () => { |
There was a problem hiding this comment.
Is it possible to move these to more specific tests? With a snapshot, when something changes all that's clear is that something has changed, and it may not be clear that it's a bad thing.
It also makes debugging start with a blank slate..instead of knowing that the failure is exactly that dependent modules aren't nested under their parents, or that names aren't trimmed of the sourceRoot, you have to start with, something has changed, track down what that is and what caused it, then go read the file under test to figure out if the original intention was that dependent modules should be nested under their parents or that names are trimmed of the sourceRoot (and also are there cases when those things shouldn't happen...).
There's probably some mixture of expect()ing specific values and snapshots (with more specific test names :P) that would work best here.
There was a problem hiding this comment.
I've made a few more specific tests.
brendankenny
left a comment
There was a problem hiding this comment.
some last style nits, but LGTM!
Over to @patrickhulce
| const unusedJavascriptSummary = await UnusedJavaScriptSummary.request( | ||
| {url: scriptElement.src, scriptCoverages, bundle}, context); | ||
|
|
||
| /** @type {Node} */ |
There was a problem hiding this comment.
nit: don't need this, tsc's got your back :)
There was a problem hiding this comment.
This can change what line an error is reported on, and makes the error message less complex. You can see the difference by commenting out the name property and seeing how the error changes.
| // For every source file, apply the data to all components | ||
| // of the source path, creating nodes as necessary. | ||
| for (const [source, data] of Object.entries(sourcesData)) { | ||
| addAllNodesInSourcePath(source || `<unmapped>`, data); |
There was a problem hiding this comment.
is the <unmapped> fallback possible? It seems like things would be pretty broken if the source name was the empty string
There was a problem hiding this comment.
ah, nope. not possible.
There is an entry in sourcesWastedBytes for (unmapped) (see
will add an item in parent issue to add a node for unmapped bytes.
patrickhulce
left a comment
There was a problem hiding this comment.
much easier for me to follow now thanks for the comments :)
I assume we might end up needing to tweak as the treemap viewer app evolves but this looks great to land 🎉
| 'b.js': {resourceBytes: 100, duplicatedNormalizedModuleName: 'blah'}, | ||
| 'c.js': {resourceBytes: 100, unusedBytes: 50}, | ||
| }); | ||
| expect(rootNode).toMatchObject( |
|
Thanks Brendan and Patrick! |
part of #11254
Example of
TreemapData: https://gist.github.com/connorjclark/2c30fc7be0e6cbfd205a4a78964226dfSize in LHR, after removing fullpage screenshots from experimental: