Skip to content

core(gather): new computed artifact, js-bundles#10078

Merged
devtools-bot merged 60 commits into
masterfrom
computed-artifact-bundle-analysis
Jan 28, 2020
Merged

core(gather): new computed artifact, js-bundles#10078
devtools-bot merged 60 commits into
masterfrom
computed-artifact-bundle-analysis

Conversation

@connorjclark

@connorjclark connorjclark commented Dec 7, 2019

Copy link
Copy Markdown
Collaborator

For usage examples, see #10064

  • Pulls in CDT source map impl. I minimized the global scope pollution, but there remains a String.prototype.asParsedURL Actually seems like ParsedURL is not important, so I removed it.
  • Colocate ScriptElement, SourceMap, NetworkRecord, CDT map, and source sizes.
  • The CDT map / sizes info for each individual bundle is lazily calculated

Maybe just name it Bundles?

Comment thread lighthouse-core/lib/cdt/SDK.js
`);
});

it('handles faulty maps', async () => {

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's tricky to create "meaningful" bad source maps, so I just mutate a bunch of things.

Comment thread package.json Outdated
"browserify": "^16.2.3",
"bundlesize": "^0.17.2",
"chalk": "^2.4.1",
"chrome-devtools-frontend": "^1.0.708769",

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi this hasn't been published in a month

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for the heads up. fixed.

1.0.723630 is today's ToT

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

diff between the two: https://diff.googleplex.com/#key=qMBBFZxro5yG
(nothing we care about)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fyi i am on 1.0.723630 rn just didnt' update the package.json semever

@connorjclark connorjclark changed the title core(bundle-analysis) - new artifact gatherers(bundle-analysis) - new computed artifact Dec 9, 2019
@connorjclark connorjclark changed the title gatherers(bundle-analysis) - new computed artifact core(gather) - new computed artifact bundle-analysis Dec 9, 2019
@vercel vercel Bot temporarily deployed to Preview January 13, 2020 23:25 Inactive
Comment thread build/build-cdt-lib.js
// Complicated expressions are hard detect with the TS lib, so instead work with the raw code.
const rawCodeToReplace = {
// Similar to the reason for removing `url += Common.UIString('? [sm]')`.
// The entries in `.mappings` should not have their url property modified.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'll be a little weird to have devtools' POV slightly different here, but looking at the spec and a few examples... I don't see a great reason why the sources' URLs should be relative to the sourcemap file's URL.

so maybe we end up also removing this in devtools.

Comment thread types/artifacts.d.ts
map?: undefined;
}

export interface Bundle {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i was going to say we don't want the same data (ScriptElement) reported in two separate artifacts... but then realized Bundle is a computed artifact.

at some point we should separate the types of our computed artifacts from our gathered artifacts. tbh kinda weird that they are all in the same spot.

@paulirish paulirish left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good to go!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants