-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
β¨π Upgrade closure compiler to v20190301 #21618
Conversation
At long last, this PR is ready for preliminary review. We are now at a point where Sending this out to all TLs and those familiar with closure compiler. Feel free to assign directories / files to others on your team. For easy reviewing, I've split off the changes into separate commits as far as possible. |
Woohoo! Makes me super happy that we are so close to head! |
Good news: Build looks good in multi-pass mode with all tests passing (link) and no visual diffs (link) Paging @erwinmombay to see if I'm missing something obvious that's causing the single-pass failures. Manual testing confirms that the single-pass build isn't working. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few questions while Single Pass is being worked on.
Great work @rsimha!
@kristoferbaxter Thanks for keeping me honest with your comments. I've left responses to each of them, and will let you resolve them when you're satisfied that there's no better alternative. Now on to dealing with the single-pass errors... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Toooo many files...
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
Outdated
Show resolved
Hide resolved
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
Outdated
Show resolved
Hide resolved
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
Outdated
Show resolved
Hide resolved
extensions/amp-apester-media/0.1/test/test-amp-apester-media.js
Outdated
Show resolved
Hide resolved
extensions/amp-story-auto-ads/0.1/test/test-amp-story-auto-ads.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed all comments by @jridgewell.
extensions/amp-ad-network-doubleclick-impl/0.1/amp-ad-network-doubleclick-impl.js
Outdated
Show resolved
Hide resolved
The canary cut for the AMP Conf release has been pushed as the 1904091426070 tag. It is now safe to merge this PR. (/cc @choumx who is release on-duty this week) Once @aghassemi and @newmuis approve the final changes, I'll do another full round of testing and then merge this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the work here to move this forward.
This PR is now ready to merge. For posterity, here is a list of things I've reviewed / verified / tested. Code review:
Local tests:
Travis tests:
AMP Release schedule
|
@rsimha Probably needs to be rolled back :( It updated the third_party code improperly. E.g. |
@dvoytenko Thanks for your comment. We can revert the changes to I'll approve and merge your partial revert ASAP. |
This PR upgrades the version of closure compiler used to minify the AMP runtime to v20190301.
Highlights:
compiler.jar
with a custom built version based on v20190301goog.requireType
or TSimport type
Β google/closure-compiler#3041 is fixed)compiler-and-tests.jar
with a custom built version based on v20190301runner.jar
after fixing code and tests to be compatible with latest compiler code, and to correctly strip out asserts@type
annotations in AMP source codeamp-story
andamp-story-auto-ads
For posterity, here is a list of things I've reviewed / verified / tested.
Code review:
Local tests:
gulp check-types
to make sure there are no type errorsgulp dist --fortesting
to make sure minified multi-pass build is healthygulp serve --compiled
and manually tested several components in multi-pass modegulp dist --fortesting --single_pass --pseudo_names
to make sure minified single-pass build is healthygulp serve --compiled
and manually tested several components in single-pass modegulp test --local-changes --headless
to make sure all unit tests affected by this PR are greenTravis tests:
presubmit
,lint
,dep-check
, andcheck-types
) are green (link)AMP Release schedule
Partial fix for #18748
Follow up to #18552
Follow up to #18609
Follow up to #18794
Follow up to #19449
Follow up to #20056