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(legacy-javascript): new audit #10303

Merged
merged 129 commits into from
Feb 19, 2020
Merged

core(legacy-javascript): new audit #10303

merged 129 commits into from
Feb 19, 2020

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Feb 6, 2020

(previous PR #6730 has too many comments, so here's a new one).

New Audit: legacy-javascript

Goal: Encourage developers to serve modern code to modern browsers.

Design Doc

This audit fails if legacy code is found in any first party code. The recommended action is to use the module/nomodule pattern, in addition to @babel/preset-modules (instead of @babel/preset-env).

This first pass does not add this audit to the default config.

Legacy code, defined

Legacy code contains either polyfills or transformed code for features that modern browsers don't need. "modern browser" is defined as a browser that support ES modules. The relevant polyfills / transforms are in this spreadsheet.

Polyfills overwrite native prototypes in regular patterns (String.prototype.startsWith = ..., Object.defineProperty(String.prototype, 'startsWith', ...) or via common libraries (core-js). The patterns that emerge are fairly detectable with a regex. The major focus was on detecting core-js polyfills, although some effort was made to detect simple ways of polyfilling.

Only some of the transforms are detectable. I went with 3 that were simple and common.

Woah, regex. Does it even work?

I wrote a tool to create a bunch of babel projects, varying on individual polyfills / transforms. The results of the tool are also checked into git. All that takes too long to run in CI, so it's just there as a manual verification. Any updates to the audit should also run that tool and check for changes in precision.

See it

http://misc-hoten.surge.sh/lh-legacy-javascript-pr/www.wikipedia.org_2020-02-06_13-20-38.report.html
http://misc-hoten.surge.sh/lh-legacy-javascript-pr/www.wix.com_2020-02-06_13-27-56.report.html

Follow up work

  1. Fix layout for subRows
  2. Use third-party-web
  3. Use source maps
  4. web.dev article
  5. Enable by default
  6. Estimate savings to make an opportunity

Runtime cost

node lighthouse-core/scripts/compare-timings.js --name my-collection --collect -n 3 --lh-flags='--only-audits=legacy-javascript' --urls https://www.coursehero.com/sg https://www.wix.com https://www.wikipedia.org
node lighthouse-core/scripts/compare-timings.js --name my-collection --summarize --measure-filter 'legacy-javascript'

image

@connorjclark
Copy link
Collaborator Author

resolved another false positive found from running on paul's site.

Here's what Wikipedia gives

URL: https://www.wikipedia.org/portal/wikipedia.org/assets/js/index-c1cb7f1287.js
Polyfills: 2

Array.prototype.indexOf at 0:1753
indexOf||(Array.prototype.indexOf=function(e,t){var n;if(null==this)throw new TypeError('"

Function.prototype.bind at 0:2580
pe.bind||(Function.prototype.bind=function(e){if("function"!=typeof this)throw new TypeErr

Should ignore those too, I think.

@connorjclark
Copy link
Collaborator Author

https://www.redditstatic.com/mweb2x/vendors~Mweb.15c8b87d53fe9156dd3a.js

ctrl + f "node_modules"

never seen that in production before (full module paths)
image

@patrickhulce
Copy link
Collaborator

@connorjclark you're planning on adding "waiting4reviewer" with a specific call to action when you feel you're blocked on us again, correct?

@connorjclark
Copy link
Collaborator Author

Smidge better UX for the empty table problem when all polyfills are 3p (doesn't need designer or anything maybe it's just that we don't enable the filter by default?)

I think the notApplicable change handles this, right?

@patrickhulce
Copy link
Collaborator

I think the notApplicable change handles this, right?

Is the table still empty without a message once you expand the notapplicable audit? That seems to be an equivalent problem to the passing audit with an empty table.

@connorjclark
Copy link
Collaborator Author

NVM, I had strange expectations for what notApplicable does :)

fixed the empty table thing. also, noticed that the row counting logic was wrong - it assumed that each row would have only one url item type. that is not true, so I changed it to count trs.

@connorjclark
Copy link
Collaborator Author

anything else reviewers @patrickhulce @paulirish ?

score: foundSignalInFirstPartyCode ? 0 : 1,
notApplicable: !foundSignalInFirstPartyCode,
extendedInfo: {
signalCount,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

should this be numericValue and "unitless"?

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMO if it isn't immediately clear from the unit + name of the audit what the numericValue represents (CLS, dom-size, opportunity, etc) then it should live in extendedInfo, so I like it as-is

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

LGTM! 🎉🎉

@patrickhulce patrickhulce removed their assignment Feb 18, 2020
@patrickhulce
Copy link
Collaborator

Aight @paulirish speak now or forever hold your peace :)

@connorjclark connorjclark merged commit fa70748 into master Feb 19, 2020
@connorjclark connorjclark deleted the ext-polys branch February 19, 2020 03:21
@connorjclark connorjclark mentioned this pull request Feb 21, 2020
16 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants