Roll forward CC upgrade with fixes (#18552) by dreamofabear · Pull Request #18609 · ampproject/amphtml

dreamofabear · 2018-10-08T20:23:28Z

New changes:

Remove node_modules/ path from js_module_root option. It caused silent bundling failure with imports leaving undefined Closure module path vars e.g. module$path$to$foo.
Document method for removing missing type paths in swg.js.
Fix react-dates issue [1] by injecting it post-compile instead of a JS import in amp-date-picker.js and removed obsolete "erwinmHack" export hack. :)
Fix a couple new type errors in amp-date-picker.js.

[1] CC v20171112 appears to DCE some of browserify's module loading boilerplate in third_party/react-dates/bundle.js. Specifically:

"moment": [function(require, module, exports) {
    //! moment.js
    (function(global, factory) {
        // This outer ternary is DCE'd by CC v20171112, but not in v20170409.
        typeof exports === 'object' && typeof module !== 'undefined' 
            ? module.exports = factory() 
            : typeof define === 'function' && define.amd 
                ? define(factory) 
                : global.moment = factory()
    }(this, (function() {
        'use strict'; ...

This reverts commit 6fd3044.

erwinmombay · 2018-10-08T20:27:38Z

build-system/get-dep-graph.js

+    // checkVars: Demote "variable foo is undeclared" errors.
+    // moduleLoad: Demote "module not found" errors to ignore missing files
+    //     in type declarations in the swg.js bundle.
+    jscomp_warning: ['checkVars', 'moduleLoad'],


did we need this for both compile and check-type tasks? was wondering if we could make it conditional

Shouldn't we be consistent between both?

@choumx ideally. i just remembered the scenario you ran into originally with multi-pass, where the compilation worked but check-types failed. was that more an inconsistency with another option?

ads/adventive.js

build-system/get-dep-graph.js

build-system/tasks/compile.js

build-system/tasks/dep-check.js

kristoferbaxter

Left a few minor comments that likely are worth addressing in a followup.

The only item with a slightly higher concern is transpiling node_modules, but the number of items AMP uses from node_modules is small.

dreamofabear · 2018-10-08T20:49:18Z

Thanks for the reviews!

dreamofabear · 2018-10-08T20:53:35Z

Looks like the upgrade increased bundle size. :(

81.55KB -> 82.1KB

The original PR might not have exhibited this due to the node_modules/ bundling issue.

dreamofabear · 2018-10-10T00:33:32Z

gulpfile.js

+    const newSource = [
+      wrapperOpen, reactDates, file.substr(firstLineBreak + 1),
+    ].join('\n');
+    fs.writeFileSync(destFilePath, newSource, 'utf8');


@erwinmombay @cvializ FYI this is how I'm solving it for multi-pass.

@choumx did this actually work for single pass? i dont think single pass reaches this code path from the testing ive been doing on master. just double checking

I haven't tested it. This technique is certainly part of the solution for single-pass. We still need to either (1) add externs or (2) change all invocations into react, moment, etc. in AMP code to either use bracket notation.

dreamofabear · 2018-10-10T04:06:06Z

Tested the normal push build steps locally (full compile and test etc.) and everything looks good after some final fixes. Merging! 🤞

* Revert "Revert CC upgrade. (ampproject#18600)" This reverts commit 6fd3044. * Remove node_modules/ from js_module_root. * Fix swg.js again. * Increase bundle size cap to 82.2. * Inject react-dates bundle post-compile instead of importing in code. * Typecast react-dates imports in amp-date-picker. * Bump bundle-size. * Restore --js_module_root for 1-pass. * Remove obsolete 'erwinmHack'.

William Chou added 3 commits October 8, 2018 16:17

Revert "Revert CC upgrade. (ampproject#18600)"

873bd3b

This reverts commit 6fd3044.

Remove node_modules/ from js_module_root.

9ccc3ea

Fix swg.js again.

f3c2bde

dreamofabear requested a review from erwinmombay October 8, 2018 20:23

googlebot added the cla: yes label Oct 8, 2018

erwinmombay reviewed Oct 8, 2018

View reviewed changes

kristoferbaxter reviewed Oct 8, 2018

View reviewed changes

ads/adventive.js Show resolved Hide resolved

kristoferbaxter reviewed Oct 8, 2018

View reviewed changes

build-system/get-dep-graph.js Outdated Show resolved Hide resolved

kristoferbaxter reviewed Oct 8, 2018

View reviewed changes

build-system/tasks/compile.js Show resolved Hide resolved

kristoferbaxter reviewed Oct 8, 2018

View reviewed changes

build-system/tasks/dep-check.js Show resolved Hide resolved

kristoferbaxter approved these changes Oct 8, 2018

View reviewed changes

Increase bundle size cap to 82.2.

7684408

dreamofabear mentioned this pull request Oct 9, 2018

amp-date-picker: Incompatibilities in bundling of react-dates #18632

Closed

2 tasks

erwinmombay approved these changes Oct 9, 2018

View reviewed changes

William Chou added 2 commits October 9, 2018 20:16

Inject react-dates bundle post-compile instead of importing in code.

55fc821

Merge branch 'master' into cc-upgrade-remix

d0a2c43

dreamofabear commented Oct 10, 2018

View reviewed changes

William Chou added 4 commits October 9, 2018 20:45

Typecast react-dates imports in amp-date-picker.

0c4539f

Bump bundle-size.

9deed0a

Restore --js_module_root for 1-pass.

3572a5d

Remove obsolete 'erwinmHack'.

38bb126

dreamofabear merged commit ad9c05e into ampproject:master Oct 10, 2018

dreamofabear deleted the cc-upgrade-remix branch October 10, 2018 04:06

rsimha mentioned this pull request Oct 17, 2018

✨🏗 Upgrade Closure Compiler to v20180101 #18794

Merged

rsimha mentioned this pull request Nov 26, 2018

✨🏗 Upgrade closure compiler to v20181028 #19449

Closed

rsimha mentioned this pull request Dec 21, 2018

✨🏗 [WIP] Upgrade closure compiler to v20181210 #20056

Closed

rsimha mentioned this pull request Mar 28, 2019

✨🏗 Upgrade closure compiler to v20190301 #21618

Merged

rsimha mentioned this pull request May 22, 2019

🏗✨ Upgrade closure compiler to v20190513 #22446

Merged

rsimha mentioned this pull request Jul 18, 2019

🏗✨ Upgrade closure compiler to v20190709 #23417

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Roll forward CC upgrade with fixes (#18552)#18609

Roll forward CC upgrade with fixes (#18552)#18609
dreamofabear merged 10 commits intoampproject:masterfrom
dreamofabear:cc-upgrade-remix

dreamofabear commented Oct 8, 2018 •

edited

Loading

Uh oh!

erwinmombay Oct 8, 2018 •

edited

Loading

Uh oh!

dreamofabear Oct 8, 2018

Uh oh!

erwinmombay Oct 8, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kristoferbaxter left a comment

Uh oh!

dreamofabear commented Oct 8, 2018

Uh oh!

dreamofabear commented Oct 8, 2018

Uh oh!

dreamofabear Oct 10, 2018

Uh oh!

erwinmombay Oct 18, 2018

Uh oh!

dreamofabear Oct 18, 2018

Uh oh!

dreamofabear commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

dreamofabear commented Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New changes:

Uh oh!

erwinmombay Oct 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

erwinmombay Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kristoferbaxter left a comment

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Oct 8, 2018

Uh oh!

dreamofabear commented Oct 8, 2018

Uh oh!

dreamofabear Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

erwinmombay Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

dreamofabear Oct 18, 2018

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Oct 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dreamofabear commented Oct 8, 2018 •

edited

Loading

erwinmombay Oct 8, 2018 •

edited

Loading