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

Turn on closure compiler collapse properties. #2972

Merged
merged 1 commit into from
Apr 23, 2016
Merged

Turn on closure compiler collapse properties. #2972

merged 1 commit into from
Apr 23, 2016

Conversation

cramforce
Copy link
Member

@cramforce cramforce commented Apr 22, 2016 •
edited
Loading

Boom.

jridgewell
jridgewell reviewed Apr 22, 2016
View reviewed changes
test/size.txt
max | min | gzip | file
--- | --- | --- | ---
9.85 kB | 3.82 kB | alp-v0.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you re-run sizes?

Copy link
Member Author

Choose a reason for hiding this comment

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

On it. Need to run gulp clean Takes a while.

Copy link
Member Author

Choose a reason for hiding this comment

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

The metrics are correct, though.

@cramforce cramforce force-pushed the collapse-properties branch from 8ed844f to 0f23987 Compare April 22, 2016 22:05
@erwinmombay
Copy link
Member

@cramforce still seeing the disconnect i was seeing in #2951 with collapsProperties on v0.js (which is why i excluded it). didnt want to merge it and break testing for everybody

@erwinmombay erwinmombay mentioned this pull request Apr 22, 2016
Closed
@cramforce cramforce force-pushed the collapse-properties branch 2 times, most recently from 03cc053 to 4a0d9c4 Compare April 23, 2016 00:04
@cramforce
Copy link
Member Author

Alright, excited to see what travis says. Worked around the PJs issue.

@cramforce
Turn on closure compiler collapse properties.
fe5e298 
Boom.

The only apparent problem was related to handling of the default export from PJs, which needed both some local changes and a version bump for PJS.
@cramforce cramforce force-pushed the collapse-properties branch from 4a0d9c4 to fe5e298 Compare April 23, 2016 00:21
@cramforce
Copy link
Member Author

Green build :)

@erwinmombay
Copy link
Member

@cramforce LGTM!

@cramforce
Copy link
Member Author

This will need some baking :)

@cramforce cramforce merged commit 4bc06b7 into ampproject:master Apr 23, 2016
@cramforce cramforce deleted the collapse-properties branch April 23, 2016 00:45
@erwinmombay
Copy link
Member

@cramforce good timing since we're only doing canary next week

@cramforce
Copy link
Member Author

+1

On Fri, Apr 22, 2016 at 5:58 PM, erwin mombay notifications@github.com
wrote:

@cramforce https://github.com/cramforce good timing since we're only
doing canary next week

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#2972 (comment)

@rsimha rsimha mentioned this pull request Oct 17, 2018
Merged
kristoferbaxter
kristoferbaxter reviewed Nov 7, 2018
View reviewed changes
test/size.txt
614.81 kB | 157.73 kB | 43.68 kB | v0.js / amp.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Looking good here if the results are stable. ~9% reduction.

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

@kristoferbaxter kristoferbaxter kristoferbaxter left review comments

Assignees

@erwinmombay erwinmombay

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cramforce @erwinmombay @kristoferbaxter @jridgewell