β¨π Upgrade closure compiler to v20190301#21618
β¨π Upgrade closure compiler to v20190301#21618rsimha merged 60 commits intoampproject:masterfrom rsimha:2019-03-22-ClosureCompiler
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. |
kristoferbaxter
left a comment
There was a problem hiding this comment.
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... |
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
rsimha
left a comment
There was a problem hiding this comment.
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. |
aghassemi
left a comment
There was a problem hiding this comment.
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.jarwith a custom built version based on v20190301goog.requireTypeor TSimport typeΒ google/closure-compiler#3041 is fixed)compiler-and-tests.jarwith a custom built version based on v20190301runner.jarafter fixing code and tests to be compatible with latest compiler code, and to correctly strip out asserts@typeannotations in AMP source codeamp-storyandamp-story-auto-adsFor posterity, here is a list of things I've reviewed / verified / tested.
Code review:
Local tests:
gulp check-typesto make sure there are no type errorsgulp dist --fortestingto make sure minified multi-pass build is healthygulp serve --compiledand manually tested several components in multi-pass modegulp dist --fortesting --single_pass --pseudo_namesto make sure minified single-pass build is healthygulp serve --compiledand manually tested several components in single-pass modegulp test --local-changes --headlessto 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