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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 5 commits into from
Jun 5, 2020
Merged

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

merged 5 commits into from
Jun 5, 2020

Conversation

connorjclark
Copy link
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
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 minus the most important byte-themed debate of our century ;)

@connorjclark
Copy link
Collaborator Author

Smoke test only fails to run in CI ...

@connorjclark connorjclark reopened this Jun 4, 2020
@stephengardner
Copy link

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

@patrickhulce
Copy link
Collaborator

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

@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
Copy link

Curious when this hits the live site? Thanks!

@stephengardner
Copy link

Hi team, any final word on this? Thanks!

@connorjclark
Copy link
Collaborator Author

connorjclark commented Jun 14, 2020 •
edited
Loading

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
Copy link

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

@patrickhulce
Copy link
Collaborator

@stephengardner see #11025

@stephengardner
Copy link

@stephengardner see #11025

Thanks @patrickhulce, following

@francescodicaprio
Copy link

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
Copy link
Collaborator Author

connorjclark commented Jun 15, 2022 •
edited
Loading

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
Copy link

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

@brendankenny brendankenny brendankenny left review comments

@patrickhulce patrickhulce patrickhulce approved these changes

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