🚀 babel/terser: rename all amp privates with sentinel suffix (#36143)

build-system/babel-config/minified-config.js

@@ -31,6 +31,7 @@ function getMinifiedConfig() {
     './build-system/babel-plugins/babel-plugin-transform-inline-isenumvalue',
     './build-system/babel-plugins/babel-plugin-transform-fix-leading-comments',
     './build-system/babel-plugins/babel-plugin-transform-promise-resolve',
+    './build-system/babel-plugins/babel-plugin-transform-rename-privates',
     '@babel/plugin-transform-react-constant-elements',
     reactJsxPlugin,
     (argv.esm || argv.sxg) &&

build-system/babel-plugins/babel-plugin-transform-rename-privates/index.js

@@ -0,0 +1,65 @@
+/**
+ * Renames all of AMP's class properties to have a unique suffix, `AMP_PRIVATE_`.
+ * This acts as an indicator for terser to mangle the names even when combined with 3p files.
+ *
+ * @param {babel} babel
+ * @interface {babel.PluginPass}
+ * @return {babel.PluginObj}
+ */
+module.exports = function (babel) {
+  const {types: t} = babel;
+
+  /**
+   * Adds trailing AMP_PRIVATE_ suffix to private identifiers.
+   * @param {string} field
+   * @return {function(*,*):void}
+   */
+  function renamePrivate(field) {
+    return function (path, state) {
+      if (!isAmpSrc(state)) {
+        return;
+      }
+      if (path.node.computed) {
+        return;
+      }
+
+      const key = path.get(field);
+      if (!key.isIdentifier()) {
+        return;
+      }
+
+      const {name} = key.node;
+      if (name.endsWith('_AMP_PRIVATE_')) {
+        return;
+      }
+
+      if (!name.endsWith('_')) {
+        return;
+      }
+      key.replaceWith(t.identifier(`${name}AMP_PRIVATE_`));
+    };
+  }
+
+  return {
+    visitor: {
+      Method: renamePrivate('key'),
+      Property: renamePrivate('key'),
+      MemberExpression: renamePrivate('property'),
+      OptionalMemberExpression: renamePrivate('property'),
+    },
+  };
+};
+
+/**
+ * @param {*} state
+ * @return {boolean}
+ */
+function isAmpSrc(state) {
+  const filename = state.file.opts.filenameRelative;
+  if (!filename) {
+    throw new Error('Cannot use plugin without providing a filename');
+  }
+  return !(
+    filename.startsWith('node_modules') || filename.startsWith('third_party')
+  );
+}

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/fixtures/transform/rename-amp-privates/input.js

@@ -0,0 +1,67 @@
+const obj = {
+  test_: 1,
+  method_() {},
+  get getter_() {},
+  set setter_(v) {},
+  shorthand_,
+
+  [test_]: 1,
+  [method_]() {},
+  get [getter_]() {},
+  set [setter_](v) {},
+
+  'test_': 1,
+  'method_'() {},
+  get 'getter_'() {},
+  set 'setter_'(v) {},
+};
+
+class Instance {
+  test_ = 1;
+  method_() {}
+  get getter_() {}
+  set setter_(v) {}
+
+  [test_] = 1;
+  [method_]() {}
+  get [getter_]() {}
+  set [setter_](v) {}
+
+  'test_' = 1
+  'method_'() {}
+  get 'getter_'() {}
+  set 'setter_'(v) {}
+}
+
+class Static {
+  static test_ = 1;
+  static method_() {}
+  static get getter_() {}
+  static set setter_(v) {}
+
+  static [test_] = 1;
+  static [method_]() {}
+  static get [getter_]() {}
+  static set [setter_](v) {}
+
+  static 'test_' = 1
+  static 'method_'() {}
+  static get 'getter_'() {}
+  static set 'setter_'(v) {}
+}
+
+foo.bar_;
+foo[bar_];
+foo['bar_'];
+
+foo?.bar_;
+foo?.[bar_];
+foo?.['bar_'];
+
+deep.foo?.bar_;
+deep.foo?.[bar_];
+deep.foo?.['bar_'];
+
+deep?.foo.bar_;
+deep?.foo.[bar_];
+deep?.foo.['bar_'];

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/fixtures/transform/rename-amp-privates/options.json

@@ -0,0 +1,7 @@
+{
+  "plugins": [
+    "../../../.."
+  ],
+  "sourceType": "module",
+  "filenameRelative": "src/foo.js"
+}

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/fixtures/transform/rename-amp-privates/output.mjs

@@ -0,0 +1,94 @@
+const obj = {
+  test_AMP_PRIVATE_: 1,
+
+  method_AMP_PRIVATE_() {},
+
+  get getter_AMP_PRIVATE_() {},
+
+  set setter_AMP_PRIVATE_(v) {},
+
+  shorthand_AMP_PRIVATE_: shorthand_,
+  [test_]: 1,
+
+  [method_]() {},
+
+  get [getter_]() {},
+
+  set [setter_](v) {},
+
+  'test_': 1,
+
+  'method_'() {},
+
+  get 'getter_'() {},
+
+  set 'setter_'(v) {}
+
+};
+
+class Instance {
+  test_AMP_PRIVATE_ = 1;
+
+  method_AMP_PRIVATE_() {}
+
+  get getter_AMP_PRIVATE_() {}
+
+  set setter_AMP_PRIVATE_(v) {}
+
+  [test_] = 1;
+
+  [method_]() {}
+
+  get [getter_]() {}
+
+  set [setter_](v) {}
+
+  'test_' = 1;
+
+  'method_'() {}
+
+  get 'getter_'() {}
+
+  set 'setter_'(v) {}
+
+}
+
+class Static {
+  static test_AMP_PRIVATE_ = 1;
+
+  static method_AMP_PRIVATE_() {}
+
+  static get getter_AMP_PRIVATE_() {}
+
+  static set setter_AMP_PRIVATE_(v) {}
+
+  static [test_] = 1;
+
+  static [method_]() {}
+
+  static get [getter_]() {}
+
+  static set [setter_](v) {}
+
+  static 'test_' = 1;
+
+  static 'method_'() {}
+
+  static get 'getter_'() {}
+
+  static set 'setter_'(v) {}
+
+}
+
+foo.bar_AMP_PRIVATE_;
+foo[bar_];
+foo['bar_'];
+foo?.bar_AMP_PRIVATE_;
+foo?.[bar_];
+foo?.['bar_'];
+deep.foo?.bar_AMP_PRIVATE_;
+deep.foo?.[bar_];
+deep.foo?.['bar_'];
+deep?.foo.bar_AMP_PRIVATE_;
+deep?.foo[bar_];
+deep?.foo['bar_'];

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/fixtures/transform/skip-nonamp-privates/input.js

@@ -0,0 +1,9 @@
+class Foo {
+  constructor() {
+    /**
+     * @type {string}
+     * @private
+     */
+    this.hello_ = 'world';
+  }
+}

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/fixtures/transform/skip-nonamp-privates/options.json

@@ -0,0 +1,9 @@
+{
+  "plugins": [
+    [
+      "../../../.."
+    ]
+  ],
+  "sourceType": "module",
+  "filenameRelative": "node_modules/foo.js"
+}

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/fixtures/transform/skip-nonamp-privates/output.mjs

@@ -0,0 +1,10 @@
+class Foo {
+  constructor() {
+    /**
+     * @type {string}
+     * @private
+     */
+    this.hello_ = 'world';
+  }
+
+}

build-system/babel-plugins/babel-plugin-transform-rename-privates/test/index.js

@@ -0,0 +1,3 @@
+const runner = require('@babel/helper-plugin-test-runner').default;
+
+runner(__dirname);

build-system/common/esbuild-babel.js

@@ -111,10 +111,12 @@ function getFileBabelOptions(babelOptions, filename) {
     babelOptions = {...babelOptions, plugins};
   }
 
+  // The amp runner automatically sets cwd to the `amphtml` directory.
+  const root = process.cwd();
   return {
     ...babelOptions,
     filename,
-    filenameRelative: path.basename(filename),
+    filenameRelative: path.relative(root, filename),
   };
 }
 

build-system/tasks/helpers.js

@@ -462,9 +462,7 @@ async function esbuildCompile(srcDir, srcFilename, destDir, options) {
     let map = result.outputFiles.find(({path}) => path.endsWith('.map')).text;
 
     if (options.minify) {
-      ({code, map} = await minify(code, map, {
-        mangle: !compiledFile && options.mangle,
-      }));
+      ({code, map} = await minify(code, map));
       map = await massageSourcemaps(map, options);
     }
 
@@ -533,12 +531,17 @@ const nameCache = {};
  *
  * @param {string} code
  * @param {string} map
- * @param {{mangle: boolean}} options
  * @return {!Promise<{code: string, map: *, error?: Error}>}
  */
-async function minify(code, map, {mangle} = {mangle: false}) {
+async function minify(code, map) {
   const terserOptions = {
-    mangle: {},
+    mangle: {
+      properties: {
+        regex: '_AMP_PRIVATE_$',
+        // eslint-disable-next-line google-camelcase/google-camelcase
+        keep_quoted: /** @type {'strict'} */ ('strict'),
+      },
+    },
     compress: {
       // Settled on this count by incrementing number until there was no more
       // effect on minification quality.
@@ -551,21 +554,9 @@ async function minify(code, map, {mangle} = {mangle: false}) {
     },
     sourceMap: {content: map},
     module: !!argv.esm,
+    nameCache,
   };
-
-  // Enabling property mangling requires disabling two other optimization.
-  // - Should not mangle quoted properties (often used for cross-binary purposes)
-  // - Should not convert computed properties into regular property definition
-  if (mangle) {
-    // eslint-disable-next-line google-camelcase/google-camelcase
-    terserOptions.mangle.properties = {keep_quoted: 'strict', regex: '_$'};
-    terserOptions.nameCache = nameCache;
-
-    // TODO: uncomment once terser bugs related to these are fixed
-    // https://github.com/terser/terser/pull/1058
-    terserOptions.compress.computed_props = false; // eslint-disable-line google-camelcase/google-camelcase
-    terserOptions.compress.properties = false;
-  }
+  // TS complains if defined inline, since it sees type `string` but needs type ("strict" | boolean).
 
   const minified = await terser.minify(code, terserOptions);
   return {code: minified.code ?? '', map: minified.map};