core(duplicated-javascript): new audit #10314

connorjclark · 2020-02-09T06:15:50Z

One of a few features I'm extracting from the bundle-analysis (#10064) branch.

In draft b/c no tests.

patrickhulce

I am super excited to see this land in default :D awesome awesome stuff!

I've lost track between PRs but is there a set of scripts you have for pulling down the coursehero examples? would be awesome to one day get some smoketests with our own webpack build that we can test out newer versions of webpack, make sure we still handle them well, etc.

lighthouse-core/audits/byte-efficiency/bundle-duplication.js

lighthouse-core/computed/javascript-duplication.js

patrickhulce · 2020-03-03T17:05:39Z

lighthouse-core/test/audits/byte-efficiency/bundle-duplication-test.js

+        {src: 'https://example.com/coursehero-bundle-2.js', content: bundleData2.content},
+      ],
+    };
+    const results = await BundleDuplicationAudit.audit_(artifacts, [], context);


would love to get 1 test where we exercise the providedWastedBytesByUrl too, maybe create some fake bundles for our fixture trace?

don't see how to do this ... computeWasteWithTTIGraph just spits out a number. does that method need to be decomposed?

I just mean test the results from audit() not just audit_() at least once

patrickhulce · 2020-03-03T17:09:25Z

lighthouse-core/test/audits/byte-efficiency/bundle-duplication-test.js

+              "https://example.com/coursehero-bundle-1.js",
+              "https://example.com/coursehero-bundle-2.js",
+            ],
+            "wastedBytes": 10519,


example of the folder thing I'd love to see node_modules/@babel/runtime and node_modules/lodash-es split out as rows :)

ah, here's the clarification i needed

you are suggesting aggregating all the modules matching node_modules/(@.*)?/(.*)/**/* into one row?

yeah exactly 👍

I have concerns about doing that - consider lodash. One bundle might need lodash/deep, another lodash/get. Pretend each is pretty big. We'd then be falsely reporting that there's duplication across those two bundles. Using different modules from the same package in different bundles should be allowed.

Also, I just think it's simpler to keep this in terms of modules.

Maybe there's a misunderstanding then. We wouldn't be falsely reporting any duplicates with what I'm proposing. This is exclusively for UI presentation in how we aggregate the already identified duplicates modules.

In the scenario that both lodash/deep and lodash/get are duplicated in different bundles, it seems pretty reasonable to say that "lodash" is duplicated. It at least gives the correct actionable advice. But the one real world situation we're analyzing here is even way more clear cut. Right now we have the completely unactionable "Other" when we could at least pull out @babel/runtime and lodash-es

Got it! I'm cool with this - even though it loses some amount specificity, knowing exactly which modules are duplicated should be possible with the treemap viz I have planned for duplicated modules. so let's do this.

lighthouse-core/audits/byte-efficiency/bundle-duplication.js

patrickhulce · 2020-03-05T21:41:26Z

lighthouse-core/audits/byte-efficiency/bundle-duplication.js

+      const networkRecord = networkRecords.find(n => n.url === url);
+      const script = artifacts.ScriptElements.find(script => script.src === url);
+      if (!networkRecord || !script || script.content === null) {
+        // ?


Suggested change

// ?

// this should never happen because we found the content from scripts that were bundles with source maps

could they have been inline scripts with a source map comment though? maybe we should fallback to the main resource for estimation which is what we did normally for estimateTransferSize (in unused css for example)

yeah that's exactly what we should do for inline bundles.

lighthouse-core/audits/byte-efficiency/bundle-duplication.js

patrickhulce · 2020-03-05T21:47:13Z

lighthouse-core/audits/byte-efficiency/bundle-duplication.js

+      {key: 'wastedBytes', valueType: 'bytes', granularity: 0.05, label: str_(i18n.UIStrings.columnWastedBytes)},
+    ];
+
+    // TODO: show warning somewhere if no source maps.


ah if we gotta wire it let's just punt then, this has grown enough :)

lighthouse-core/computed/javascript-duplication.js

patrickhulce · 2020-03-05T21:51:20Z

lighthouse-core/computed/javascript-duplication.js

+  static _shouldIgnoreSource(source) {
+    // Ignore bundle overhead.
+    if (source.includes('webpack/bootstrap')) return true;
+    if (source.includes('(webpack)/buildin')) return true;


just curious did you take a look at any sites built with parcel for their overhead names?

https://lhci-canary.herokuapp.com/app/projects/d1e4b15c-e644-4552-b136-e975f486a2ce/dashboard should have sourcemaps and multiple bundles for future examples if you need one

Co-Authored-By: Patrick Hulce <[email protected]>

patrickhulce

LGTM

patrickhulce · 2020-03-06T21:11:25Z

lighthouse-core/audits/byte-efficiency/bundle-duplication.js

+const UIStrings = {
+  /** Imperative title of a Lighthouse audit that tells the user to remove duplicate JavaScript from their code. This is displayed in a list of audit titles that Lighthouse generates. */
+  title: 'Remove duplicate modules in JavaScript bundles',
+  /** Description of a Lighthouse audit that tells the user *why* they should defer loading any content in CSS that isn’t needed at page load. This is displayed after a user expands the section to see more. No word length limits. 'Learn More' becomes link text to additional documentation. */


Suggested change

/** Description of a Lighthouse audit that tells the user *why* they should defer loading any content in CSS that isn’t needed at page load. This is displayed after a user expands the section to see more. No word length limits. 'Learn More' becomes link text to additional documentation. */

/** Description of a Lighthouse audit that tells the user *why* they should remote duplicate JavaScript from their scripts. This is displayed after a user expands the section to see more. No word length limits. 'Learn More' becomes link text to additional documentation. */

patrickhulce · 2020-03-06T21:14:06Z

lighthouse-core/computed/javascript-duplication.js

+  static _shouldIgnoreSource(source) {
+    // Ignore bundle overhead.
+    if (source.includes('webpack/bootstrap')) return true;
+    if (source.includes('(webpack)/buildin')) return true;


interesting thanks for looking into ti! we can followup here if we end up seeing this in practice later

patrickhulce · 2020-03-06T21:16:06Z

lighthouse-core/test/audits/byte-efficiency/bundle-duplication-test.js

+    const context = {settings, computedCache: new Map()};
+    const results = await BundleDuplicationAudit.audit(artifacts, context);
+
+    // Without the `wastedBytesByUrl` this would be zero because the items don't define a url.


connorjclark · 2020-03-06T22:57:34Z

@patrickhulce suggested some renames:

computed artifact => ModuleDuplication
audit => duplicated-javascript

connorjclark added 2 commits February 8, 2020 21:07

bundledup

209443b

update

61a37de

googlebot added the cla: yes label Feb 9, 2020

connorjclark mentioned this pull request Feb 9, 2020

core: add temporary config for source map features #10311

Merged

connorjclark added 2 commits February 13, 2020 15:01

Merge remote-tracking branch 'origin/master' into bundle-dupe

777ee52

config

9d6d626

vercel bot deployed to Preview February 13, 2020 23:02 View deployment

Merge remote-tracking branch 'origin/master' into bundle-dupe

d36f530

vercel bot deployed to Preview February 21, 2020 05:34 View deployment

Merge remote-tracking branch 'origin/master' into bundle-dupe

bf93536

vercel bot deployed to Preview February 29, 2020 07:16 View deployment

add test for computed artifact

8320059

vercel bot deployed to Preview February 29, 2020 22:52 View deployment

fix size lookup

8fc34b3

vercel bot deployed to Preview March 2, 2020 18:07 View deployment

complex maps

8b2671d

vercel bot deployed to Preview March 2, 2020 18:31 View deployment

connorjclark added 4 commits March 2, 2020 10:32

comment

cb04763

lint

4f7e466

Merge remote-tracking branch 'origin/master' into bundle-dupe

e63e9d8

add audit test

6dcd4de

connorjclark requested a review from patrickhulce March 2, 2020 18:48

vercel bot deployed to Preview March 2, 2020 18:48 View deployment

connorjclark marked this pull request as ready for review March 2, 2020 18:48

connorjclark requested a review from a team as a code owner March 2, 2020 18:48

devtools-bot assigned patrickhulce Mar 2, 2020

devtools-bot added the waiting4reviewer label Mar 2, 2020

patrickhulce suggested changes Mar 3, 2020

View reviewed changes

patrickhulce added waiting4committer and removed waiting4reviewer labels Mar 3, 2020

types

1f50f45

vercel bot deployed to Preview March 5, 2020 21:10 View deployment

fix

75714f9

vercel bot deployed to Preview March 5, 2020 21:14 View deployment

transfer

01acbd4

vercel bot deployed to Preview March 5, 2020 21:26 View deployment

connorjclark commented Mar 5, 2020

View reviewed changes

lighthouse-core/audits/byte-efficiency/bundle-duplication.js Outdated Show resolved Hide resolved

patrickhulce reviewed Mar 5, 2020

View reviewed changes

test

e98ce9b

vercel bot deployed to Preview March 5, 2020 22:35 View deployment

connorjclark added 2 commits March 5, 2020 14:56

fix sizing for inline

978b052

i18n

453fcb5

vercel bot temporarily deployed to Preview March 5, 2020 22:58 Inactive

Update lighthouse-core/computed/javascript-duplication.js

83d0c6a

Co-Authored-By: Patrick Hulce <[email protected]>

vercel bot deployed to Preview March 5, 2020 22:59 View deployment

patrickhulce approved these changes Mar 6, 2020

View reviewed changes

comment

962c507

vercel bot deployed to Preview March 6, 2020 22:54 View deployment

rename

759ccf1

vercel bot temporarily deployed to Preview March 6, 2020 23:03 Inactive

i18n

f228fa4

vercel bot deployed to Preview March 6, 2020 23:03 View deployment

connorjclark changed the title ~~core(bundle-duplication): new audit~~ core(duplicated-javascript): new audit Mar 6, 2020

connorjclark added land-when-ci-is-green and removed waiting4committer labels Mar 6, 2020

devtools-bot merged commit ea9d226 into master Mar 6, 2020

devtools-bot deleted the bundle-dupe branch March 6, 2020 23:28

devtools-bot removed the land-when-ci-is-green label Mar 6, 2020

connorjclark added the new_audit label Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

core(duplicated-javascript): new audit #10314

core(duplicated-javascript): new audit #10314

connorjclark commented Feb 9, 2020

patrickhulce left a comment

patrickhulce Mar 3, 2020

connorjclark Mar 5, 2020

patrickhulce Mar 5, 2020

connorjclark Mar 5, 2020

patrickhulce Mar 3, 2020

connorjclark Mar 3, 2020

patrickhulce Mar 3, 2020

connorjclark Mar 5, 2020

patrickhulce Mar 5, 2020

connorjclark Mar 5, 2020

patrickhulce Mar 5, 2020

connorjclark Mar 5, 2020

patrickhulce Mar 5, 2020

patrickhulce Mar 5, 2020

patrickhulce left a comment

patrickhulce Mar 6, 2020

patrickhulce Mar 6, 2020

patrickhulce Mar 6, 2020

connorjclark commented Mar 6, 2020

	// ?
	// this should never happen because we found the content from scripts that were bundles with source maps

	/** Description of a Lighthouse audit that tells the user why they should defer loading any content in CSS that isn’t needed at page load. This is displayed after a user expands the section to see more. No word length limits. 'Learn More' becomes link text to additional documentation. */
	/** Description of a Lighthouse audit that tells the user why they should remote duplicate JavaScript from their scripts. This is displayed after a user expands the section to see more. No word length limits. 'Learn More' becomes link text to additional documentation. */

core(duplicated-javascript): new audit #10314

core(duplicated-javascript): new audit #10314

Conversation

connorjclark commented Feb 9, 2020

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

connorjclark commented Mar 6, 2020