Skip to content

core(unused-javascript): increase threshold to 20KiB#10906

Merged
connorjclark merged 5 commits into
masterfrom
unusedjsthresh
Jun 5, 2020
Merged

core(unused-javascript): increase threshold to 20KiB#10906
connorjclark merged 5 commits into
masterfrom
unusedjsthresh

Conversation

@connorjclark

@connorjclark connorjclark commented Jun 3, 2020

Copy link
Copy Markdown
Collaborator

The threshold is too low.

Background: ampproject/amphtml#28638

@connorjclark connorjclark requested a review from a team as a code owner June 3, 2020 23:25
@connorjclark connorjclark requested review from patrickhulce and removed request for a team June 3, 2020 23:25
@connorjclark connorjclark mentioned this pull request Jun 3, 2020
Closed
patrickhulce
patrickhulce approved these changes Jun 3, 2020
View reviewed changes

@patrickhulce patrickhulce left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM minus the most important byte-themed debate of our century ;)

Comment thread lighthouse-core/audits/byte-efficiency/unused-javascript.js Outdated
@connorjclark @patrickhulce
Update lighthouse-core/audits/byte-efficiency/unused-javascript.js
643b86f 
Co-authored-by: Patrick Hulce <patrick.hulce@gmail.com>
@connorjclark

connorjclark commented Jun 4, 2020

Copy link
Copy Markdown
Collaborator Author

Smoke test only fails to run in CI ...

@connorjclark connorjclark reopened this Jun 4, 2020
@stephengardner

stephengardner commented Jun 4, 2020

Copy link
Copy Markdown

Cheering this one on 👏👏 thanks for the quick action @connorjclark

@patrickhulce

patrickhulce commented Jun 4, 2020

Copy link
Copy Markdown
Collaborator

Wow failed to produce a report at all that's really weird...just for a threshold change?

brendankenny
brendankenny reviewed Jun 4, 2020
View reviewed changes
Comment thread lighthouse-cli/test/smokehouse/test-definitions/byte-efficiency/byte-config.js Outdated
@connorjclark @brendankenny
Update lighthouse-cli/test/smokehouse/test-definitions/byte-efficienc…
92e3ae5 
…y/byte-config.js

Co-authored-by: Brendan Kenny <bckenny@gmail.com>
@connorjclark connorjclark mentioned this pull request Jun 5, 2020
Merged
@connorjclark connorjclark merged commit 0002a58 into master Jun 5, 2020
@connorjclark connorjclark deleted the unusedjsthresh branch June 5, 2020 23:02
@stephengardner

stephengardner commented Jun 6, 2020

Copy link
Copy Markdown

Curious when this hits the live site? Thanks!

@stephengardner

stephengardner commented Jun 14, 2020

Copy link
Copy Markdown

Hi team, any final word on this? Thanks!

@connorjclark

connorjclark commented Jun 14, 2020 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

We have a scheduled release 24th. Subscribe to the releases notification on this repo to be alerted

@patrickhulce patrickhulce mentioned this pull request Jun 17, 2020
Closed
@patrickhulce patrickhulce mentioned this pull request Jun 23, 2020
Closed
@stephengardner

stephengardner commented Jun 25, 2020

Copy link
Copy Markdown

Has the release been delayed? Is there a new estimated release date?
image
Thanks

@patrickhulce

patrickhulce commented Jun 25, 2020

Copy link
Copy Markdown
Collaborator

@stephengardner see #11025

@stephengardner

stephengardner commented Jun 25, 2020

Copy link
Copy Markdown

@stephengardner see #11025

Thanks @patrickhulce, following

@francescodicaprio

francescodicaprio commented Jun 15, 2022

Copy link
Copy Markdown

Upvoting this. It reports as unused javascript even the GTAG for Google Analytics, which is well known as something that must be loaded in the head as Google itself suggests.

@connorjclark

connorjclark commented Jun 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Collaborator Author

This has been fixed for two years now @francescodicaprio . If you have a new issue, please open a new issue and fill out the issue template.

@francescodicaprio

francescodicaprio commented Jun 15, 2022

Copy link
Copy Markdown

My apologies, I thought merged stuff got closed, didn’t see the merged tag.
Anyway, it may exclude under 20k of stuff, but still reports as bad the gtag by Google. It’s funny seeing Google page speed reporting as bad a Google script that must be in the head.

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

Reviewers

2 more reviewers

@brendankenny brendankenny brendankenny left review comments

@patrickhulce patrickhulce patrickhulce approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@patrickhulce patrickhulce

Labels

cla: yes waiting4reviewer

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@connorjclark @stephengardner @patrickhulce @francescodicaprio @brendankenny @googlebot @devtools-bot