Run Closure on non-minified prod builds, too

In facebook#26446 we started publishing non-minified versions of our production
build artifacts, along with source maps, for easier debugging of React
when running in production mode.

The way it's currently set up is that these builds are generated
*before* Closure compiler has run. Which means it's missing many of
the optimizations that are in the final build, like dead
code elimination.

This PR changes the build process to run Closure on the non-minified
production builds, too, by moving the sourcemap generation to later in
the pipeline.

The non-minified builds will still preserve the original symbol names,
and we'll use Prettier to add back whitespace. This is the exact same
approach we've been using for years to generate production builds
for Meta.

The idea is that the only difference between the minified and non-
minified builds is whitespace and symbol mangling. The semantic
structure of the program should be identical.

To implement this, I disabled symbol mangling when running Closure
compiler. Then, in a later step, the symbols are mangled by Terser.
This is when the source maps are generated.

Author: acdlite

package.json

@@ -94,6 +94,7 @@
     "shelljs": "^0.8.5",
     "signedsource": "^2.0.0",
     "targz": "^1.0.1",
+    "terser": "^5.30.3",
     "through2": "^3.0.1",
     "tmp": "^0.1.0",
     "typescript": "^3.7.5",

scripts/rollup/build.js

@@ -24,6 +24,7 @@ const Packaging = require('./packaging');
 const {asyncRimRaf} = require('./utils');
 const codeFrame = require('@babel/code-frame');
 const Wrappers = require('./wrappers');
+const minify = require('terser').minify;
 
 const RELEASE_CHANNEL = process.env.RELEASE_CHANNEL;
 
@@ -487,137 +488,58 @@ function getPlugins(
           );
         },
       },
-      // License and haste headers for artifacts with sourcemaps
-      // For artifacts with sourcemaps we apply these headers
-      // before passing sources to the Closure compiler, which will be building sourcemaps
-      needsSourcemaps && {
-        name: 'license-and-signature-header-for-artifacts-with-sourcemaps',
-        renderChunk(source) {
-          return Wrappers.wrapWithLicenseHeader(
-            source,
-            bundleType,
-            globalName,
-            filename,
-            moduleType
-          );
-        },
-      },
-      // Apply dead code elimination and/or minification.
-      // closure doesn't yet support leaving ESM imports intact
+      // For production builds, compile with Closure. We do this even for the
+      // "non-minified" production builds because Closure is much better at
+      // minification than what most applications use. During this step, we do
+      // preserve the original symbol names, though, so the resulting code is
+      // relatively readable.
+      //
+      // For the minified builds, the names will be mangled later.
+      //
+      // We don't bother with sourcemaps at this step. The sourcemaps we publish
+      // are only for whitespace and symbol renaming; they don't map back to
+      // before Closure was applied.
       needsMinifiedByClosure &&
-        closure(
-          {
-            compilation_level: 'SIMPLE',
-            language_in: 'ECMASCRIPT_2020',
-            language_out:
-              bundleType === NODE_ES2015
-                ? 'ECMASCRIPT_2020'
-                : bundleType === BROWSER_SCRIPT
-                ? 'ECMASCRIPT5'
-                : 'ECMASCRIPT5_STRICT',
-            emit_use_strict:
-              bundleType !== BROWSER_SCRIPT &&
-              bundleType !== ESM_PROD &&
-              bundleType !== ESM_DEV,
-            env: 'CUSTOM',
-            warning_level: 'QUIET',
-            source_map_include_content: true,
-            use_types_for_optimization: false,
-            process_common_js_modules: false,
-            rewrite_polyfills: false,
-            inject_libraries: false,
-            allow_dynamic_import: true,
-
-            // Don't let it create global variables in the browser.
-            // https://github.com/facebook/react/issues/10909
-            assume_function_wrapper: !isUMDBundle,
-            renaming: !shouldStayReadable,
-          },
-          {needsSourcemaps}
-        ),
-      // Add the whitespace back if necessary.
-      shouldStayReadable &&
+        closure({
+          compilation_level: 'SIMPLE',
+          language_in: 'ECMASCRIPT_2020',
+          language_out:
+            bundleType === NODE_ES2015
+              ? 'ECMASCRIPT_2020'
+              : bundleType === BROWSER_SCRIPT
+              ? 'ECMASCRIPT5'
+              : 'ECMASCRIPT5_STRICT',
+          emit_use_strict:
+            bundleType !== BROWSER_SCRIPT &&
+            bundleType !== ESM_PROD &&
+            bundleType !== ESM_DEV,
+          env: 'CUSTOM',
+          warning_level: 'QUIET',
+          source_map_include_content: true,
+          use_types_for_optimization: false,
+          process_common_js_modules: false,
+          rewrite_polyfills: false,
+          inject_libraries: false,
+          allow_dynamic_import: true,
+
+          // Don't let it create global variables in the browser.
+          // https://github.com/facebook/react/issues/10909
+          assume_function_wrapper: !isUMDBundle,
+
+          // Don't rename symbols (variable names, functions, etc). This will
+          // be handled in a later step.
+          renaming: false,
+        }),
+      needsMinifiedByClosure &&
+        // Add the whitespace back
         prettier({
           parser: 'flow',
           singleQuote: false,
           trailingComma: 'none',
           bracketSpacing: true,
         }),
-      needsSourcemaps && {
-        name: 'generate-prod-bundle-sourcemaps',
-        async renderChunk(minifiedCodeWithChangedHeader, chunk, options, meta) {
-          // We want to generate a sourcemap that shows the production bundle source
-          // as it existed before Closure Compiler minified that chunk, rather than
-          // showing the "original" individual source files. This better shows
-          // what is actually running in the app.
-
-          // Use a path like `node_modules/react/cjs/react.production.min.js.map` for the sourcemap file
-          const finalSourcemapPath = options.file.replace('.js', '.js.map');
-          const finalSourcemapFilename = path.basename(finalSourcemapPath);
-          const outputFolder = path.dirname(options.file);
-
-          // Read the sourcemap that Closure wrote to disk
-          const sourcemapAfterClosure = JSON.parse(
-            fs.readFileSync(finalSourcemapPath, 'utf8')
-          );
-
-          // Represent the "original" bundle as a file with no `.min` in the name
-          const filenameWithoutMin = filename.replace('.min', '');
-          // There's _one_ artifact where the incoming filename actually contains
-          // a folder name: "use-sync-external-store-shim/with-selector.production.js".
-          // The output path already has the right structure, but we need to strip this
-          // down to _just_ the JS filename.
-          const preMinifiedFilename = path.basename(filenameWithoutMin);
-
-          // CC generated a file list that only contains the tempfile name.
-          // Replace that with a more meaningful "source" name for this bundle
-          // that represents "the bundled source before minification".
-          sourcemapAfterClosure.sources = [preMinifiedFilename];
-          sourcemapAfterClosure.file = filename;
-
-          // All our code is considered "third-party" and should be ignored by default.
-          sourcemapAfterClosure.ignoreList = [0];
-
-          // We'll write the pre-minified source to disk as a separate file.
-          // Because it sits on disk, there's no need to have it in the `sourcesContent` array.
-          // That also makes the file easier to read, and available for use by scripts.
-          // This should be the only file in the array.
-          const [preMinifiedBundleSource] =
-            sourcemapAfterClosure.sourcesContent;
-
-          // Remove this entirely - we're going to write the file to disk instead.
-          delete sourcemapAfterClosure.sourcesContent;
-
-          const preMinifiedBundlePath = path.join(
-            outputFolder,
-            preMinifiedFilename
-          );
-
-          // Write the original source to disk as a separate file
-          fs.writeFileSync(preMinifiedBundlePath, preMinifiedBundleSource);
-
-          // Overwrite the Closure-generated file with the final combined sourcemap
-          fs.writeFileSync(
-            finalSourcemapPath,
-            JSON.stringify(sourcemapAfterClosure)
-          );
-
-          // Add the sourcemap URL to the actual bundle, so that tools pick it up
-          const sourceWithMappingUrl =
-            minifiedCodeWithChangedHeader +
-            `\n//# sourceMappingURL=${finalSourcemapFilename}`;
-
-          return {
-            code: sourceWithMappingUrl,
-            map: null,
-          };
-        },
-      },
-      // License and haste headers for artifacts without sourcemaps
-      // Primarily used for FB-artifacts, which should preserve specific format of the header
-      // Which potentially can be changed by Closure minification
-      !needsSourcemaps && {
-        name: 'license-and-signature-header-for-artifacts-without-sourcemaps',
+      {
+        name: 'license-and-signature-header',
         renderChunk(source) {
           return Wrappers.wrapWithLicenseHeader(
             source,
@@ -628,6 +550,87 @@ function getPlugins(
           );
         },
       },
+      isProduction &&
+        !shouldStayReadable && {
+          name: 'mangle-symbol-names',
+          async renderChunk(code, chunk, options, meta) {
+            // Minify the code by mangling symbol names. We already ran Closure
+            // on this code, so stuff like dead code elimination and inlining
+            // has already happened. This step is purely to rename the symbols,
+            // which we asked Closure to preserve.
+            //
+            // The only reason this is a separate step from Closure is so we
+            // can publish non-mangled versions of the code for easier debugging
+            // in production. We also publish sourcemaps that map back to the
+            // non-mangled code (*not* the pre-Closure code).
+
+            const outputFolder = path.dirname(options.file);
+
+            // Represent the "original" bundle as a file with no `.min` in the name
+            const filenameWithoutMin = filename.replace('.min', '');
+            // There's _one_ artifact where the incoming filename actually contains
+            // a folder name: "use-sync-external-store-shim/with-selector.production.js".
+            // The output path already has the right structure, but we need to strip this
+            // down to _just_ the JS filename.
+            const preMinifiedFilename = path.basename(filenameWithoutMin);
+            const preMinifiedBundlePath = path.join(
+              outputFolder,
+              preMinifiedFilename
+            );
+
+            // Use a path like `node_modules/react/cjs/react.production.min.js.map` for the sourcemap file
+            const finalSourcemapPath = options.file.replace('.js', '.js.map');
+            const finalSourcemapFilename = path.basename(finalSourcemapPath);
+
+            const terserOptions = {
+              // Don't bother compressing. Closure already did that.
+              compress: false,
+              // Mangle the symbol names.
+              mangle: true,
+              toplevel: true,
+            };
+            if (needsSourcemaps) {
+              terserOptions.sourceMap = {
+                // Used to set the `file` field in the sourcemap
+                filename: filename,
+                // Used to set `# sourceMappingURL=` in the compiled code
+                url: finalSourcemapFilename,
+              };
+            }
+
+            const minifiedResult = await minify(
+              {[preMinifiedFilename]: code},
+              terserOptions
+            );
+
+            // Create the directory if it doesn't already exist
+            fs.mkdirSync(outputFolder, {recursive: true});
+
+            if (needsSourcemaps) {
+              const sourcemapJSON = JSON.parse(minifiedResult.map);
+
+              // All our code is considered "third-party" and should be ignored
+              // by default
+              sourcemapJSON.ignoreList = [0];
+
+              // Write the sourcemap to disk
+              fs.writeFileSync(
+                finalSourcemapPath,
+                JSON.stringify(sourcemapJSON)
+              );
+            }
+
+            // Write the original source to disk as a separate file
+            fs.writeFileSync(preMinifiedBundlePath, code);
+
+            return {
+              code: minifiedResult.code,
+              // TODO: Maybe we should use Rollup's sourcemap feature instead
+              // of writing it to disk manually?
+              map: null,
+            };
+          },
+        },
       // Record bundle size.
       sizes({
         getSize: (size, gzip) => {

scripts/rollup/plugins/closure-plugin.js

@@ -19,20 +19,16 @@ function compile(flags) {
   });
 }
 
-module.exports = function closure(flags = {}, {needsSourcemaps}) {
+module.exports = function closure(flags = {}) {
   return {
     name: 'scripts/rollup/plugins/closure-plugin',
     async renderChunk(code, chunk, options) {
       const inputFile = tmp.fileSync();
 
-      // Use a path like `node_modules/react/cjs/react.production.min.js.map` for the sourcemap file
-      const sourcemapPath = options.file.replace('.js', '.js.map');
-
       // Tell Closure what JS source file to read, and optionally what sourcemap file to write
       const finalFlags = {
         ...flags,
         js: inputFile.name,
-        ...(needsSourcemaps && {create_source_map: sourcemapPath}),
       };
 
       await writeFileAsync(inputFile.name, code, 'utf8');