Babel Following Closure Compilation #26779

kristoferbaxter · 2020-02-13T00:53:47Z

Introduces a Babel and Terser processing step post Closure Compiler invocation.

This allows us to write Babel plugins (like the one included) that modify the output of Closure generated code to take advantage of newer language features in the module build.

This PR also removes shorten-license which did not appear to work for ESM builds.

amp-owners-bot · 2020-02-13T00:53:53Z

Hey @jridgewell, these files were changed:

build-system/babel-plugins/babel-plugin-transform-fix-leading-comments/index.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/argument-function/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/argument-function/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/argument-function/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/arguments-identifier/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/arguments-identifier/options.json 
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/arguments-identifier/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/async-function/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/async-function/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/async-function/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/class-function-declaration-in-public-method/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/class-function-declaration-in-public-method/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/class-function-declaration-in-public-method/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/existing-arrow-function/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/existing-arrow-function/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/existing-arrow-function/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/existing-async-arrow-function/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/existing-async-arrow-function/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/existing-async-arrow-function/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/export-default-named-declaration/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/export-default-named-declaration/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/export-default-named-declaration/output.mjs
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/export-named-declaration/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/export-named-declaration/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/export-named-declaration/output.mjs
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/generator-function/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/generator-function/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/generator-function/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/multiple-body-statements/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/multiple-body-statements/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/multiple-body-statements/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/new-expression/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/new-expression/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/new-expression/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/no-return/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/no-return/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/no-return/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/single-return-statement/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/single-return-statement/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/single-return-statement/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/this-argument/input.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/this-argument/options.json
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/fixtures/transform-assertions/this-argument/output.js
build-system/babel-plugins/babel-plugin-transform-function-declarations/test/index.js
build-system/babel-plugins/babel-plugin-transform-minified-comments/index.js
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/fixtures/transform/licenses-tabs/input.js
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/fixtures/transform/licenses-tabs/options.json
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/fixtures/transform/licenses-tabs/output.js
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/fixtures/transform/licenses/input.js
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/fixtures/transform/licenses/options.json
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/fixtures/transform/licenses/output.js
build-system/babel-plugins/babel-plugin-transform-minified-comments/test/index.js

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

alanorozco · 2020-02-14T19:45:00Z

Are the numbers reflecting extension changes?

Diffing both I only see that v0 changes size:

-   ✔ dist/v0.mjs                                     64.38 kb   74.99 kb   228.51 kb  
+   ✔ dist/v0.mjs                                     63.85 kb   74.57 kb   231.03 kb

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

kristoferbaxter · 2020-02-14T20:30:39Z

Are the numbers reflecting extension changes?

Should be, I'll verify though since it's a surprise there was no change.

third_party/webcomponentsjs/ShadowCSS.js

samouri · 2020-02-19T19:13:10Z

Does Closure Compiler not support generating two outputs, one for module and the other for nomodule? It feels strange to compile certain parts down and then after CC finishes, transform them back up to es6.

kristoferbaxter · 2020-03-23T19:01:42Z

Closure Compiler doesn't support outputting with two different language_out versions from a single invocation.

Ideally we are not doing any transpilation with Closure, and relying on Babel for this work.

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

kristoferbaxter · 2020-03-24T02:50:47Z

@rsimha Can you approve the eslintrc change?

rsimha

Thanks for working on this, @kristoferbaxter. I've requested some changes to the compilation code. Also, I didn't see any changes to .eslintrc. Is it missing from this PR?

rsimha · 2020-03-24T15:21:17Z

build-system/compile/build.conf.js

@@ -138,8 +141,18 @@ function getJsonConfigurationPlugin() {
 * @param {!Object<string, boolean>} buildFlags
 * @return {!Array<string|!Array<string|!Object>>}
 */
-function plugins({isEsmBuild, isForTesting, isSinglePass, isChecktypes}) {
-  const applied = [...defaultPlugins(isEsmBuild || false)];
+function plugins({


FYI @rcebulko and @jridgewell, since this code is being refactored in #27161.

build-system/compile/compile.js

build-system/tasks/helpers.js

build-system/compile/compile.js

rsimha · 2020-03-24T15:35:19Z

build-system/compile/compile.js

+ * @param {boolean} isEsmBuild
+ * @return {!Promise}
+ */
+function postClosureBabel(directory, isEsmBuild) {


A couple questions, since it looks like we're now doing babel transforms before and after closure compilation:

Any chance the two can be consolidated at some point?

The pre-closure babel transforms run on all files at once. This currently prevents us from enabling lazy-building for minified builds. Would it be possible to do the pre-closure transforms per-file as well, and add them to the chain in the main compile function?

Obviously, both these items needn't be addressed in this PR.

Any chance the two can be consolidated at some point?

Not likely, the transforms executed post closure are expected to optimize the output of Closure Compiled code for our specific usecase. There might be times where we need to retain information from a pre-closure scan/transform and use it afterwards too.

The pre-closure babel transforms run on all files at once. This currently prevents us from enabling lazy-building for minified builds. Would it be possible to do the pre-closure transforms per-file as well, and add them to the chain in the main compile function?

Sounds like an interesting experiment for a future PR. Don't have a compelling reason why this must be done all at once.

Agree, I've talked to @erwinmombay about a plan to address pre-closure transforms in a new PR.

This is being done in #27426.

kristoferbaxter · 2020-03-24T15:51:07Z

@rsimha Sorry, it was .eslintignore.

Looking at the other comments now.

…ddressed additional output from unformatted comments

kristoferbaxter · 2020-03-25T00:52:35Z

build-system/babel-plugins/babel-plugin-transform-fix-leading-comments/index.js

@@ -23,7 +23,7 @@ module.exports = function() {
      Statement(path) {
        const {node} = path;
        const {trailingComments} = node;
-        if (!trailingComments) {
+        if (!trailingComments || trailingComments.length <= 0) {


This transform was executing on empty array lengths, addressed since this PR also includes a new transform doing something very similar.

just for my own knowledge, can the comments property actually be null?

erwinmombay · 2020-03-25T15:11:16Z

@kristoferbaxter read through it again and LGTM

rsimha

LGTM after a few minor comments are addressed. Thanks!

build-system/compile/compile.js

build-system/compile/post-closure-babel.js

build-system/compile/shorten-license.js

src/examiner/examiner.js

kristoferbaxter · 2020-03-25T16:55:05Z

@samouri or @choumx Can you take a look at the change for examiner.js. Your approval is required.

There is a review comment explaining the change.

dreamofabear

Whoa that's a large diff. :)

src/examiner/examiner.js changes LGTM.

rsimha

Latest changes LGTM.

Paging @jridgewell who added strict mode directives a long time ago (#9575), for comment on whether there's any reason we can't remove all the use stricts in our code.

dreamofabear · 2020-03-25T18:13:30Z

Modules are strict by default so moving our infra source to ES6 would allow that. Though I think we'd still need transpilation to run browser tests on IE.

Somewhat related: AMP currently runs in sloppy mode (#19380), so we probably need to check that "strict by default" in module build doesn't break anything that relies on sloppy mode behavior.

kristoferbaxter · 2020-03-25T18:17:58Z

If we convert the entire node source code to ESM, and load it with either .mjs extensions or {type:"modules"} then we would be strict by default.

There is also the option to invoke node with --use_strict, but that has broken several times in the past on LTS releases so it's not recommended.

jridgewell · 2020-03-26T03:47:34Z

build-system/compile/post-closure-babel.js

+const conf = require('./build.conf');
+const fs = require('fs-extra');
+const path = require('path');
+const resorcery = require('@jridgewell/resorcery');


This should be @ampproject/remapping.

Looks like the entire codebase is using your previously personal named scope version. Sounds like a good change to make wholesale.

jridgewell · 2020-03-26T03:48:50Z

build-system/compile/post-closure-babel.js

+
+  return {
+    compressed: minified.code,
+    terserMap: minified.map,


Nit: why not just return the minified object as is? code and map properties are pretty standard for every build library.

We can, but the method consuming it was renaming the values anyway. Since it's only intended to be used within this module, I renamed the output to the expected form.

jridgewell · 2020-03-26T03:50:38Z

build-system/compile/post-closure-babel.js

+      remapped.toString()
+    );
+
+    return next(null, file);


Did we verify the sourcemaps coming out of this? Please verify by loading it into https://justin.ridgewell.name/source-map-visualization/

I did verify before submitting the first PR. Will check again.

jridgewell · 2020-03-26T03:51:11Z

build-system/babel-plugins/babel-plugin-transform-minified-comments/index.js

+      Statement(path) {
+        const {node} = path;
+        const {trailingComments} = node;
+        if (!trailingComments || trailingComments.length <= 0) {


Shouldn't this be looking at leadingComments, too?

jridgewell · 2020-03-26T03:53:08Z

build-system/babel-plugins/babel-plugin-transform-minified-comments/index.js

+          ...comment,
+          value: comment.value.replace(/\n +/g, `\n`),
+        }));
+        next.addComments('leading', formattedComments);


Why put the comments onto a new node in this plugin? The babel-plugin-transform-fix-leading-comments should already be taking care of this.

Feel free to try within the codebase, the existing transform did not properly format the comments in a way that would work with terser.

jridgewell · 2020-03-26T04:08:26Z

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

+  isNewedInProgramScope: (t, path) => {
+    const {name} = path.get('id').node;
+    let isNewed = false;
+    path


We can use the binding directly to skip a full file traversal:

const binding = path.scope.getBinding(name); const isNewed = binding.referencePaths.some(p => p.parentPath.isNewExpression());

jridgewell · 2020-03-26T04:13:27Z

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

+      });
+    return isNewed;
+  },
+};


There are more cases we should be checking for. Essentially, if the binding is used in any way that is not a simple CallExpression, then it is not safe to convert.

const binding = path.scope.getBinding(name); const isSafeToTransform = binding.referencePaths.every(p => p.parentPath.isCallExpression({ callee: p.node })); });

This would subsume the isExported and isNewed checks.

jridgewell · 2020-03-26T04:17:51Z

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

+  },
+};
+
+function createVariableDeclaration(t, path) {


Nit: Everything should be inside the module.exports function, so that you don't need to keep passing around the t variable. The function will only ever be invoked once, so it won't slow anything down.

jridgewell · 2020-03-26T04:20:34Z

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

+  return {
+    name: 'safe-arrows',
+    pre() {
+      this.bailoutCount = {...BAIL_OUT_CONDITIONS};


Can we delete all of this? It's just doing extra work which makes it more difficult to understand the transformer.

jridgewell · 2020-03-26T04:23:45Z

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js

+          console /*OK*/
+            .log(`Success for function ${path.get('id').node.name}.`);
+        }
+        path.replaceWith(createVariableDeclaration(t, path));


This is not 100% safe. FunctionDeclarations are available to be used at any time in their enclosing scope, even before the program would reach the line that define the function. Replacing it with a variable declaration does not preserve that.

The only way to make this safe is to find the enclosing block, and insert this variable as the first statement of the body.

googlebot added the cla: yes label Feb 13, 2020

rsimha mentioned this pull request Feb 13, 2020

[owners] Disable "notify owner" comments on draft PRs ampproject/amp-github-apps#708

Closed

kristoferbaxter marked this pull request as ready for review February 14, 2020 00:22

amp-owners-bot bot requested review from rsimha and samouri February 14, 2020 00:22

kristoferbaxter requested a review from erwinmombay February 14, 2020 00:23

kristoferbaxter changed the title ~~Babel post~~ Babel Following Closure Compilation Feb 14, 2020

kevinkimball reviewed Feb 14, 2020

View reviewed changes

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js Outdated Show resolved Hide resolved

kristoferbaxter mentioned this pull request Feb 14, 2020

🚀 Transform Function Declarations #23358

Closed

erwinmombay reviewed Feb 14, 2020

View reviewed changes

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js Show resolved Hide resolved

erwinmombay reviewed Feb 14, 2020

View reviewed changes

third_party/webcomponentsjs/ShadowCSS.js Outdated Show resolved Hide resolved

kristoferbaxter requested a review from erwinmombay March 23, 2020 21:56

erwinmombay reviewed Mar 23, 2020

View reviewed changes

build-system/babel-plugins/babel-plugin-transform-function-declarations/index.js Show resolved Hide resolved

erwinmombay approved these changes Mar 23, 2020

View reviewed changes

kristoferbaxter added 9 commits March 24, 2020 08:31

Move closure to await

98febd3

Broken Gulp

8d17f25

wrong directory output

a7e8ebb

remove unused deps

976cec6

Working as intended now

afa9f2f

Only apply ESM post closure transforms to ESM code

526b9f6

Remove broken shorten-license

9bf827c

Move from const to let, to allow more variable folding

11a48dd

conflict in build conf

f92ae01

kristoferbaxter added 2 commits March 24, 2020 08:36

merge conflict

82adc56

merge conflicts

40e49ad

rsimha suggested changes Mar 24, 2020

View reviewed changes

merge conflicts

da7fa21

kristoferbaxter added 2 commits March 24, 2020 09:45

Seperate post closure babel pass from compile

2c5e40e

Refactored post closure transform methods based on PR feedback, and a…

382d82b

…ddressed additional output from unformatted comments

kristoferbaxter requested a review from rsimha March 25, 2020 00:51

kristoferbaxter commented Mar 25, 2020

View reviewed changes

Merge branch 'master' of github.com:ampproject/amphtml into babel-post

cd4311e

rsimha approved these changes Mar 25, 2020

View reviewed changes

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

build-system/compile/post-closure-babel.js Outdated Show resolved Hide resolved

build-system/compile/shorten-license.js Show resolved Hide resolved

src/examiner/examiner.js Show resolved Hide resolved

kristoferbaxter added 2 commits March 25, 2020 09:42

add antiquated use strict directive to antiquated node patterns

b66b19e

Removed unused depdendency

d6bb63c

kristoferbaxter requested a review from dreamofabear March 25, 2020 16:59

dreamofabear approved these changes Mar 25, 2020

View reviewed changes

rsimha approved these changes Mar 25, 2020

View reviewed changes

dreamofabear mentioned this pull request Mar 25, 2020

module build should be run under strict mode #23734

Closed

kristoferbaxter merged commit 6f90ee3 into ampproject:master Mar 25, 2020

kristoferbaxter deleted the babel-post branch March 25, 2020 19:44

jridgewell reviewed Mar 26, 2020

View reviewed changes

rsimha mentioned this pull request Mar 26, 2020

🏗🚀 Speed up pre-closure babel transforms #27426

Merged

1 task

kristoferbaxter mentioned this pull request Mar 26, 2020

🏗 Refactor babel-plugin-transform-function-declarations #27438

Merged

ampprojectbot added the PR use: In Beta / Experimental label Mar 31, 2020

rsimha mentioned this pull request Aug 26, 2020

🏗🚮 Remove unused packages #29990

Merged

rsimha mentioned this pull request Jun 1, 2021

🏗 Don't duplicate copyright comments in ESM output #34628

Merged

Babel Following Closure Compilation #26779

Babel Following Closure Compilation #26779

Conversation

kristoferbaxter commented Feb 13, 2020 • edited Loading

amp-owners-bot bot commented Feb 13, 2020 • edited Loading

alanorozco commented Feb 14, 2020

kristoferbaxter commented Feb 14, 2020

samouri commented Feb 19, 2020 • edited Loading

kristoferbaxter commented Mar 23, 2020 • edited Loading

kristoferbaxter commented Mar 24, 2020

rsimha left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsimha Mar 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristoferbaxter commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinmombay commented Mar 25, 2020

rsimha left a comment

Choose a reason for hiding this comment

kristoferbaxter commented Mar 25, 2020 • edited Loading

dreamofabear left a comment

Choose a reason for hiding this comment

rsimha left a comment

Choose a reason for hiding this comment

dreamofabear commented Mar 25, 2020

kristoferbaxter commented Mar 25, 2020

Choose a reason for hiding this comment

kristoferbaxter Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jridgewell Mar 26, 2020 • edited Loading

Choose a reason for hiding this comment

kristoferbaxter commented Feb 13, 2020 •

edited

Loading

amp-owners-bot bot commented Feb 13, 2020 •

edited

Loading

samouri commented Feb 19, 2020 •

edited

Loading

kristoferbaxter commented Mar 23, 2020 •

edited

Loading

rsimha left a comment •

edited

Loading

rsimha Mar 24, 2020 •

edited

Loading

kristoferbaxter commented Mar 25, 2020 •

edited

Loading

kristoferbaxter Mar 26, 2020 •

edited

Loading

jridgewell Mar 26, 2020 •

edited

Loading