Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 60 commits into from
Jan 28, 2020

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Dec 7, 2019

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?

lighthouse-core/lib/cdt/SDK.js Show resolved Hide resolved
`);
});

it('handles faulty maps', async () => {
Copy link
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.

package.json Outdated
@@ -103,6 +103,7 @@
"browserify": "^16.2.3",
"bundlesize": "^0.17.2",
"chalk": "^2.4.1",
"chrome-devtools-frontend": "^1.0.708769",
Copy link
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
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
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
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
// 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
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.

@@ -283,6 +290,17 @@ declare global {
map?: undefined;
}

export interface Bundle {
Copy link
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.

Copy link
Member

@paulirish paulirish left a comment

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