Move minification to the transformation phase

Reviewed By: mjesun

Differential Revision: D7670710

fbshipit-source-id: 8a55de0d3a1ee4879d21391d47fd0acd66482f44

packages/metro/src/Bundler.js

@@ -19,11 +19,6 @@ const fs = require('fs');
 const getTransformCacheKeyFn = require('./lib/getTransformCacheKeyFn');
 
 const {Cache, stableHash} = require('metro-cache');
-const {
-  toSegmentTuple,
-  fromRawMappings,
-  toBabelSegments,
-} = require('metro-source-map');
 
 import type {
   TransformedCode,
@@ -37,10 +32,7 @@ import type Module from './node-haste/Module';
 import type {BabelSourceMap} from '@babel/core';
 import type {CacheStore} from 'metro-cache';
 import type {CustomResolver} from 'metro-resolver';
-import type {
+import type {MetroSourceMap} from 'metro-source-map';
-  MetroSourceMapSegmentTuple,
-  MetroSourceMap,
-} from 'metro-source-map';
 
 type TransformOptions = {|
   +inlineRequires: {+blacklist: {[string]: true}} | boolean,
@@ -135,7 +127,6 @@ class Bundler {
     this._transformer = new Transformer({
       asyncRequireModulePath: opts.asyncRequireModulePath,
       maxWorkers: opts.maxWorkers,
-      minifierPath: opts.minifierPath,
       reporters: {
         stdoutChunk: chunk =>
           opts.reporter.update({type: 'worker_stdout_chunk', chunk}),
@@ -177,6 +168,7 @@ class Bundler {
       opts.assetExts,
       opts.assetRegistryPath,
       getTransformCacheKey,
+      opts.minifierPath,
       'experimental',
     ]).toString('binary');
 
@@ -237,25 +229,6 @@ class Bundler {
     return this._depGraphPromise;
   }
 
-  async minifyModule(
-    path: string,
-    code: string,
-    map: Array<MetroSourceMapSegmentTuple>,
-  ): Promise<{code: string, map: Array<MetroSourceMapSegmentTuple>}> {
-    const sourceMap = fromRawMappings([{code, source: code, map, path}]).toMap(
-      undefined,
-      {},
-    );
-
-    const minified = await this._transformer.minify(path, code, sourceMap);
-    const result = await this._opts.postMinifyProcess({...minified});
-
-    return {
-      code: result.code,
-      map: result.map ? toBabelSegments(result.map).map(toSegmentTuple) : [],
-    };
-  }
-
   async _cachedTransformCode(
     module: Module,
     code: ?string,
@@ -334,6 +307,7 @@ class Bundler {
         transformCodeOptions,
         this._opts.assetExts,
         this._opts.assetRegistryPath,
+        this._opts.minifierPath,
       );
     }
 

packages/metro/src/Bundler/__tests__/Bundler-test.js

@@ -12,7 +12,6 @@
 
 jest
   .setMock('jest-worker', () => ({}))
-  .setMock('metro-minify-uglify')
   .mock('image-size')
   .mock('fs', () => new (require('metro-memory-fs'))())
   .mock('os')
@@ -97,28 +96,6 @@ describe('Bundler', function() {
     expect(b._opts.platforms).toEqual(['android', 'vr']);
   });
 
-  it('should minify code using the Transformer', async () => {
-    const code = 'arbitrary(code)';
-    const id = 'arbitrary.js';
-
-    const minifiedCode = 'minified(code)';
-    const minifiedMap = {
-      version: 3,
-      file: ['minified'],
-      sources: [],
-      mappings: '',
-    };
-
-    bundler._transformer.minify = jest
-      .fn()
-      .mockReturnValue(Promise.resolve({code: minifiedCode, map: minifiedMap}));
-
-    const result = await bundler.minifyModule(id, code, []);
-
-    expect(result.code).toEqual(minifiedCode);
-    expect(result.map).toEqual([]);
-  });
-
   it('uses new cache layers when transforming if requested to do so', async () => {
     const get = jest.fn();
     const set = jest.fn();

packages/metro/src/JSTransformer.js

@@ -16,17 +16,13 @@ const {Logger} = require('metro-core');
 const debug = require('debug')('Metro:JStransformer');
 const Worker = require('jest-worker').default;
 
-import type {BabelSourceMap} from '@babel/core';
 import type {Options, TransformedCode} from './JSTransformer/worker';
 import type {LocalPath} from './node-haste/lib/toLocalPath';
-import type {MetroMinifier} from 'metro-minify-uglify';
-import type {ResultWithMap} from 'metro-minify-uglify';
 import type {DynamicRequiresBehavior} from './ModuleGraph/worker/collectDependencies';
 
 import typeof {transform as Transform} from './JSTransformer/worker';
 
 type WorkerInterface = Worker & {
-  minify: MetroMinifier,
   transform: Transform,
 };
 
@@ -45,11 +41,9 @@ module.exports = class Transformer {
   _transformModulePath: string;
   _asyncRequireModulePath: string;
   _dynamicDepsInPackages: DynamicRequiresBehavior;
-  _minifierPath: string;
 
   constructor(options: {|
     +maxWorkers: number,
-    +minifierPath: string,
     +reporters: Reporters,
     +transformModulePath: string,
     +asyncRequireModulePath: string,
@@ -59,7 +53,6 @@ module.exports = class Transformer {
     this._transformModulePath = options.transformModulePath;
     this._asyncRequireModulePath = options.asyncRequireModulePath;
     this._dynamicDepsInPackages = options.dynamicDepsInPackages;
-    this._minifierPath = options.minifierPath;
     const {workerPath = require.resolve('./JSTransformer/worker')} = options;
 
     if (options.maxWorkers > 1) {
@@ -90,19 +83,6 @@ module.exports = class Transformer {
     }
   }
 
-  async minify(
-    filename: string,
-    code: string,
-    sourceMap: BabelSourceMap,
-  ): Promise<ResultWithMap> {
-    return await this._worker.minify(
-      filename,
-      code,
-      sourceMap,
-      this._minifierPath,
-    );
-  }
-
   async transform(
     filename: string,
     localPath: LocalPath,
@@ -111,6 +91,7 @@ module.exports = class Transformer {
     options: Options,
     assetExts: $ReadOnlyArray<string>,
     assetRegistryPath: string,
+    minifierPath: string,
   ): Promise<TransformerResult> {
     try {
       debug('Started transforming file', filename);
@@ -124,6 +105,7 @@ module.exports = class Transformer {
         options,
         assetExts,
         assetRegistryPath,
+        minifierPath,
         this._asyncRequireModulePath,
         this._dynamicDepsInPackages,
       );

packages/metro/src/JSTransformer/__tests__/Transformer-test.js

@@ -23,7 +23,6 @@ describe('Transformer', function() {
   const opts = {
     asyncRequireModulePath: 'asyncRequire',
     maxWorkers: 4,
-    minifierPath: defaults.DEFAULT_METRO_MINIFIER_PATH,
     reporters: {},
     transformModulePath,
     dynamicDepsInPackages: 'reject',
@@ -79,6 +78,7 @@ describe('Transformer', function() {
       transformOptions,
       [],
       '',
+      defaults.DEFAULT_METRO_MINIFIER_PATH,
     );
 
     expect(api.transform).toBeCalledWith(
@@ -90,6 +90,7 @@ describe('Transformer', function() {
       transformOptions,
       [],
       '',
+      defaults.DEFAULT_METRO_MINIFIER_PATH,
       'asyncRequire',
       'reject',
     );

packages/metro/src/JSTransformer/worker.js

@@ -24,7 +24,11 @@ const path = require('path');
 
 const {babylon} = require('../babel-bridge');
 const {babelGenerate: generate} = require('../babel-bridge');
-const {toSegmentTuple} = require('metro-source-map');
+const {
+  fromRawMappings,
+  toBabelSegments,
+  toSegmentTuple,
+} = require('metro-source-map');
 
 import type {DynamicRequiresBehavior} from '../ModuleGraph/worker/collectDependencies';
 import type {LocalPath} from '../node-haste/lib/toLocalPath';
@@ -124,6 +128,7 @@ async function transformCode(
   options: Options,
   assetExts: $ReadOnlyArray<string>,
   assetRegistryPath: string,
+  minifierPath: string,
   asyncRequireModulePath: string,
   dynamicDepsInPackages: DynamicRequiresBehavior,
 ): Promise<Data> {
@@ -253,10 +258,27 @@ async function transformCode(
     sourceCode,
   );
 
-  const map = result.rawMappings ? result.rawMappings.map(toSegmentTuple) : [];
+  let map = result.rawMappings ? result.rawMappings.map(toSegmentTuple) : [];
+  let code = result.code;
+
+  if (options.minify) {
+    const sourceMap = fromRawMappings([
+      {code, source: sourceCode, map, path: filename},
+    ]).toMap(undefined, {});
+
+    const minified = await minifyCode(
+      filename,
+      result.code,
+      sourceMap,
+      minifierPath,
+    );
+
+    code = minified.code;
+    map = minified.map ? toBabelSegments(minified.map).map(toSegmentTuple) : [];
+  }
 
   return {
-    result: {dependencies, code: result.code, map},
+    result: {dependencies, code, map},
     sha1,
     transformFileStartLogEntry,
     transformFileEndLogEntry,
@@ -316,6 +338,5 @@ class InvalidRequireCallError extends Error {
 
 module.exports = {
   transform: transformCode,
-  minify: minifyCode,
   InvalidRequireCallError,
 };

packages/metro/src/JSTransformer/worker/__tests__/worker-test.js

@@ -12,6 +12,12 @@
 jest
   .mock('../constant-folding-plugin')
   .mock('../inline-plugin')
+  .mock('../../../lib/getMinifier', () => () => ({
+    withSourceMap: (code, map) => ({
+      code: code.replace('arbitrary(code)', 'minified(code)'),
+      map,
+    }),
+  }))
   .mock('metro-minify-uglify');
 
 const path = require('path');
@@ -33,6 +39,7 @@ describe('code transformation worker:', () => {
       },
       [],
       '',
+      'minifyModulePath',
       'asyncRequire',
       'reject',
     );
@@ -61,6 +68,7 @@ describe('code transformation worker:', () => {
       },
       [],
       '',
+      'minifyModulePath',
       'asyncRequire',
       'reject',
     );
@@ -96,6 +104,7 @@ describe('code transformation worker:', () => {
         },
         [],
         '',
+        'minifyModulePath',
         'asyncRequire',
         'reject',
       );
@@ -138,6 +147,7 @@ describe('code transformation worker:', () => {
         },
         [],
         '',
+        'minifyModulePath',
         'asyncRequire',
         'reject',
       );
@@ -181,6 +191,7 @@ describe('code transformation worker:', () => {
         },
         [],
         '',
+        'minifyModulePath',
         'asyncRequire',
         'reject',
       );
@@ -207,8 +218,38 @@ describe('code transformation worker:', () => {
       },
       [],
       '',
+      'minifyModulePath',
       'asyncRequire',
       'throwAtRuntime',
     );
   });
+
+  it('minifies the code correctly', async () => {
+    expect(
+      (await transformCode(
+        '/root/node_modules/bar/file.js',
+        `node_modules/bar/file.js`,
+        'arbitrary(code);',
+        path.join(__dirname, '../../../transformer.js'),
+        false,
+        {
+          dev: true,
+          minify: true,
+          transform: {},
+          enableBabelRCLookup: false,
+        },
+        [],
+        '',
+        'minifyModulePath',
+        'asyncRequire',
+        'throwAtRuntime',
+      )).result.code,
+    ).toBe(
+      [
+        '__d(function (global, _$$_REQUIRE, module, exports, _dependencyMap) {',
+        '  minified(code);',
+        '});',
+      ].join('\n'),
+    );
+  });
 });

packages/metro/src/Server.js

@@ -30,7 +30,6 @@ const getAbsolutePath = require('./lib/getAbsolutePath');
 const getMaxWorkers = require('./lib/getMaxWorkers');
 const getPrependedScripts = require('./lib/getPrependedScripts');
 const mime = require('mime-types');
-const mapGraph = require('./lib/mapGraph');
 const nullthrows = require('fbjs/lib/nullthrows');
 const parseCustomTransformOptions = require('./lib/parseCustomTransformOptions');
 const parsePlatformFilePath = require('./node-haste/lib/parsePlatformFilePath');
@@ -263,11 +262,7 @@ class Server {
   }
 
   async build(options: BundleOptions): Promise<{code: string, map: string}> {
-    let graphInfo = await this._buildGraph(options);
+    const graphInfo = await this._buildGraph(options);
-
-    if (options.minify) {
-      graphInfo = await this._minifyGraph(graphInfo);
-    }
 
     const entryPoint = getAbsolutePath(
       options.entryFile,
@@ -313,11 +308,7 @@ class Server {
   }
 
   async getRamBundleInfo(options: BundleOptions): Promise<RamBundleInfo> {
-    let graphInfo = await this._buildGraph(options);
+    const graphInfo = await this._buildGraph(options);
-
-    if (options.minify) {
-      graphInfo = await this._minifyGraph(graphInfo);
-    }
 
     const entryPoint = getAbsolutePath(
       options.entryFile,
@@ -450,10 +441,6 @@ class Server {
       }
     }
 
-    if (options.minify) {
-      graphInfo = await this._minifyGraph(graphInfo);
-    }
-
     return {...graphInfo, numModifiedFiles};
   }
 
@@ -493,56 +480,12 @@ class Server {
       this._deltaGraphs.set(id, graphInfo);
     }
 
-    if (options.minify) {
-      // $FlowIssue #16581373 spread of an exact object should be exact
-      delta = {
-        ...delta,
-        modified: new Map(
-          await Promise.all(
-            Array.from(delta.modified).map(async ([path, module]) => [
-              path,
-              await this._minifyModule(module),
-            ]),
-          ),
-        ),
-      };
-    }
-
     return {
       ...graphInfo,
       delta,
     };
   }
 
-  async _minifyGraph(graphInfo: GraphInfo): Promise<GraphInfo> {
-    const prepend = await Promise.all(
-      graphInfo.prepend.map(script => this._minifyModule(script)),
-    );
-    const graph = await mapGraph(graphInfo.graph, module =>
-      this._minifyModule(module),
-    );
-
-    // $FlowIssue #16581373 spread of an exact object should be exact
-    return {...graphInfo, prepend, graph};
-  }
-
-  async _minifyModule(module: Module): Promise<Module> {
-    const {code, map} = await this._bundler.minifyModule(
-      module.path,
-      module.output.code,
-      module.output.map,
-    );
-
-    return {
-      ...module,
-      output: {
-        ...module.output,
-        code,
-        map,
-      },
-    };
-  }
-
   onFileChange(type: string, filePath: string) {
     // Make sure the file watcher event runs through the system before
     // we rebuild the bundles.

packages/metro/src/Server/__tests__/Server-test.js

@@ -12,7 +12,6 @@
 
 jest
   .mock('jest-worker', () => ({}))
-  .mock('metro-minify-uglify')
   .mock('crypto')
   .mock('../symbolicate/symbolicate', () => ({
     createWorker: jest.fn().mockReturnValue(jest.fn()),
@@ -169,13 +168,6 @@ describe('processRequest', () => {
       }),
     );
 
-    Bundler.prototype.minifyModule = jest.fn().mockReturnValue(
-      Promise.resolve({
-        code: '__d(function(){minified();});',
-        map: [],
-      }),
-    );
-
     server = new Server(options);
     requestHandler = server.processRequest.bind(server);
 
@@ -307,28 +299,6 @@ describe('processRequest', () => {
     });
   });
 
-  it('calculates an incremental minified bundle', async () => {
-    await makeRequest(
-      requestHandler,
-      'mybundle.bundle?runModule=true&minify=true',
-    );
-
-    const response = await makeRequest(
-      requestHandler,
-      'mybundle.bundle?runModule=true&minify=true',
-    );
-    expect(response.statusCode).toEqual(200);
-    expect(response.body).toEqual(
-      [
-        '__d(function(){minified();});',
-        '__d(function(){minified();},0,[1],"mybundle.js");',
-        '__d(function(){minified();},1,[],"foo.js");',
-        'require(0);',
-        '//# sourceMappingURL=http://localhost:8081/mybundle.map?runModule=true&minify=true',
-      ].join('\n'),
-    );
-  });
-
   it('returns sourcemap on request of *.map', async () => {
     const response = await makeRequest(requestHandler, 'mybundle.map');
 
@@ -608,29 +578,6 @@ describe('processRequest', () => {
         reset: true,
       });
     });
-
-    it('should generate a minified delta correctly', async () => {
-      const response = await makeRequest(
-        requestHandler,
-        'index.delta?platform=ios&minify=true',
-      );
-
-      expect(JSON.parse(response.body)).toEqual({
-        id: 'XXXXX-0',
-        pre: [[-1, 'function () {require();}']],
-        delta: [
-          [0, '__d(function(){minified();},0,[1],"mybundle.js");'],
-          [1, '__d(function(){minified();},1,[],"foo.js");'],
-        ],
-        post: [
-          [
-            2,
-            '//# sourceMappingURL=http://localhost:8081/index.map?platform=ios&minify=true',
-          ],
-        ],
-        reset: true,
-      });
-    });
   });
 
   describe('/onchange endpoint', () => {

packages/metro/src/lib/__tests__/mapGraph-test.js

@@ -1,58 +0,0 @@
-/**
- * Copyright (c) 2015-present, Facebook, Inc.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- * @emails oncall+javascript_foundation
- * @format
- */
-
-'use strict';
-
-const mapGraph = require('../mapGraph');
-
-let graph;
-
-beforeEach(() => {
-  graph = {
-    dependencies: new Map([
-      ['/entryPoint', {name: 'entryPoint', id: '1'}],
-      ['/foo', {name: 'foo', id: '2'}],
-      ['/baz', {name: 'baz', id: '3'}],
-    ]),
-    entryPoints: ['/entryPoint'],
-  };
-});
-
-it('should map the passed graph when a sync function is passed', async () => {
-  const mapped = await mapGraph(graph, element => ({
-    name: '-' + element.name + '-',
-    id: parseInt(element.id, 10),
-  }));
-
-  expect(mapped.dependencies).toEqual(
-    new Map([
-      ['/entryPoint', {name: '-entryPoint-', id: 1}],
-      ['/foo', {name: '-foo-', id: 2}],
-      ['/baz', {name: '-baz-', id: 3}],
-    ]),
-  );
-  expect(mapped.entryPoints).toEqual(['/entryPoint']);
-});
-
-it('should map the passed graph when an async function is passed', async () => {
-  const mapped = await mapGraph(graph, async element => ({
-    name: '-' + element.name + '-',
-    id: parseInt(element.id, 10),
-  }));
-
-  expect(mapped.dependencies).toEqual(
-    new Map([
-      ['/entryPoint', {name: '-entryPoint-', id: 1}],
-      ['/foo', {name: '-foo-', id: 2}],
-      ['/baz', {name: '-baz-', id: 3}],
-    ]),
-  );
-  expect(mapped.entryPoints).toEqual(['/entryPoint']);
-});

packages/metro/src/lib/mapGraph.js

@@ -1,40 +0,0 @@
-/**
- * Copyright (c) 2015-present, Facebook, Inc.
- *
- * This source code is licensed under the MIT license found in the
- * LICENSE file in the root directory of this source tree.
- *
- * @flow
- * @format
- */
-
-'use strict';
-
-import type {Module} from '../DeltaBundler/traverseDependencies';
-import type {Graph} from '../DeltaBundler';
-
-/**
- * Generates a new Graph object, which has all the dependencies returned by the
- * mapping function (similar to Array.prototype.map).
- **/
-async function mapGraph(
-  graph: Graph,
-  mappingFn: Module => Promise<Module>,
-): Promise<Graph> {
-  const dependencies = new Map(
-    await Promise.all(
-      Array.from(graph.dependencies.entries()).map(async ([path, module]) => {
-        const mutated = await mappingFn(module);
-
-        return [path, mutated];
-      }),
-    ),
-  );
-
-  return {
-    dependencies,
-    entryPoints: graph.entryPoints,
-  };
-}
-
-module.exports = mapGraph;