metro-bundler: collect-dependencies: expose async deps

Summary: To determine whether segment boundaries are properly covered by async imports rather than requires, we need to get knowledge about it higher up in the stack. This changeset exposes which of the dependencies are async as an array of indices within the `dependencies` array (I'd prefer avoiding duplicating the strings because they could get inconsistent, and I don't want to have 2 separate arrays of names either because we'd have to modify a bunch of stuff across the stack to support that).

Reviewed By: davidaurelio

Differential Revision: D6220236

fbshipit-source-id: 1ee36bc7c59f7f27e089f7771a24c45c8bd57b5d

packages/metro-bundler/src/ModuleGraph/Graph.js

@@ -21,6 +21,7 @@ import type {
   LoadResult,
   Module,
   ResolveFn,
+  TransformResultDependency,
 } from './types.flow';
 
 const NO_OPTIONS = {};
@@ -40,7 +41,7 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
 
     const queue: Queue<
       {
-        id: string,
+        dependency: TransformResultDependency,
         parent: ?string,
         parentDependencyIndex: number,
         skip: ?Set<string>,
@@ -48,9 +49,9 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
       LoadResult,
       Map<?string, Module>,
     > = new Queue(
-      ({id, parent}) =>
+      ({dependency, parent}) =>
         memoizingLoad(
-          resolve(id, parent, platform, options || NO_OPTIONS),
+          resolve(dependency.name, parent, platform, options || NO_OPTIONS),
           loadOptions,
         ),
       onFileLoaded,
@@ -58,7 +59,7 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
     );
 
     const tasks = Array.from(entryPoints, (id, i) => ({
-      id,
+      dependency: {name: id, isAsync: false},
       parent: null,
       parentDependencyIndex: i,
       skip,
@@ -153,19 +154,23 @@ function onFileLoaded(
   queue,
   modules,
   {file, dependencies},
-  {id, parent, parentDependencyIndex, skip},
+  {dependency, parent, parentDependencyIndex, skip},
 ) {
   const {path} = file;
   const parentModule = modules.get(parent);
 
   invariant(parentModule, 'Invalid parent module: ' + String(parent));
-  parentModule.dependencies[parentDependencyIndex] = {id, path};
+  parentModule.dependencies[parentDependencyIndex] = {
+    id: dependency.name,
+    isAsync: dependency.isAsync,
+    path,
+  };
 
   if ((!skip || !skip.has(path)) && !modules.has(path)) {
     modules.set(path, {file, dependencies: Array(dependencies.length)});
     queue.enqueue(
-      ...dependencies.map((id, i) => ({
-        id,
+      ...dependencies.map((dependency, i) => ({
+        dependency,
         parent: path,
         parentDependencyIndex: i,
         skip,

packages/metro-bundler/src/ModuleGraph/__tests__/Graph-test.js

@@ -167,7 +167,7 @@ describe('Graph:', () => {
     resolve.stub.withArgs('entry').returns(entryPath);
     load.stub.withArgs(entryPath).returns({
       file: {path: entryPath},
-      dependencies: [id1, id2],
+      dependencies: [depOf(id1), depOf(id2)],
     });
 
     await graph(['entry'], anyPlatform, noOpts);
@@ -191,9 +191,9 @@ describe('Graph:', () => {
       .returns(entryPath);
     load.stub
       .withArgs(entryPath)
-      .returns({file: {path: entryPath}, dependencies: [id1]})
+      .returns({file: {path: entryPath}, dependencies: [depOf(id1)]})
       .withArgs(path1)
-      .returns({file: {path: path1}, dependencies: [id2]});
+      .returns({file: {path: path1}, dependencies: [depOf(id2)]});
 
     await graph(['entry'], anyPlatform, noOpts);
     expect(resolve).toBeCalledWith(id2, path1, any(String), any(Object));
@@ -207,9 +207,12 @@ describe('Graph:', () => {
     resolve.stub.callsFake(idToPath);
     load.stub
       .withArgs(idToPath('a'))
-      .returns({file: createFileFromId('a'), dependencies: ['b', 'c']})
+      .returns({
+        file: createFileFromId('a'),
+        dependencies: [depOf('b'), depOf('c')],
+      })
       .withArgs(idToPath('b'))
-      .returns({file: createFileFromId('b'), dependencies: ['c']})
+      .returns({file: createFileFromId('b'), dependencies: [depOf('c')]})
       .withArgs(idToPath('c'))
       .returns({file: createFileFromId('c'), dependencies: []});
 
@@ -247,13 +250,20 @@ describe('Graph:', () => {
         .withArgs(path)
         .returns({file: createFileFromId(id), dependencies: []});
     });
-    load.stub
-      .withArgs(idToPath('a'))
-      .returns({file: createFileFromId('a'), dependencies: ['b', 'e', 'h']});
+    load.stub.withArgs(idToPath('a')).returns({
+      file: createFileFromId('a'),
+      dependencies: ['b', 'e', 'h'].map(depOf),
+    });
 
     // load certain files later
-    const b = deferred({file: createFileFromId('b'), dependencies: ['c', 'd']});
-    const e = deferred({file: createFileFromId('e'), dependencies: ['f', 'g']});
+    const b = deferred({
+      file: createFileFromId('b'),
+      dependencies: ['c', 'd'].map(depOf),
+    });
+    const e = deferred({
+      file: createFileFromId('e'),
+      dependencies: ['f', 'g'].map(depOf),
+    });
     load.stub
       .withArgs(idToPath('b'))
       .returns(b.promise)
@@ -288,13 +298,13 @@ describe('Graph:', () => {
 
     load.stub
       .withArgs(idToPath('a'))
-      .returns({file: createFileFromId('a'), dependencies: ['b']});
+      .returns({file: createFileFromId('a'), dependencies: [depOf('b')]});
     load.stub
       .withArgs(idToPath('b'))
       .returns({file: createFileFromId('b'), dependencies: []});
     load.stub
       .withArgs(idToPath('c'))
-      .returns({file: createFileFromId('c'), dependencies: ['d']});
+      .returns({file: createFileFromId('c'), dependencies: [depOf('d')]});
     load.stub
       .withArgs(idToPath('d'))
       .returns({file: createFileFromId('d'), dependencies: []});
@@ -316,7 +326,7 @@ describe('Graph:', () => {
 
     load.stub
       .withArgs(idToPath('a'))
-      .returns({file: createFileFromId('a'), dependencies: ['b']});
+      .returns({file: createFileFromId('a'), dependencies: [depOf('b')]});
     load.stub
       .withArgs(idToPath('b'))
       .returns({file: createFileFromId('b'), dependencies: []});
@@ -342,9 +352,10 @@ describe('Graph:', () => {
         .returns({file: createFileFromId(id), dependencies: []});
     });
     ['a', 'd'].forEach(id =>
-      load.stub
-        .withArgs(idToPath(id))
-        .returns({file: createFileFromId(id), dependencies: ['b', 'c']}),
+      load.stub.withArgs(idToPath(id)).returns({
+        file: createFileFromId(id),
+        dependencies: ['b', 'c'].map(depOf),
+      }),
     );
 
     const result = await graph(['a', 'd', 'b'], anyPlatform, noOpts);
@@ -366,11 +377,11 @@ describe('Graph:', () => {
       .returns(idToPath('c'));
     load.stub
       .withArgs(idToPath('a'))
-      .returns({file: createFileFromId('a'), dependencies: ['b']})
+      .returns({file: createFileFromId('a'), dependencies: [depOf('b')]})
       .withArgs(idToPath('b'))
-      .returns({file: createFileFromId('b'), dependencies: ['c']})
+      .returns({file: createFileFromId('b'), dependencies: [depOf('c')]})
       .withArgs(idToPath('c'))
-      .returns({file: createFileFromId('c'), dependencies: ['a']});
+      .returns({file: createFileFromId('c'), dependencies: [depOf('a')]});
 
     const result = await graph(['a'], anyPlatform, noOpts);
     expect(result.modules).toEqual([
@@ -386,9 +397,12 @@ describe('Graph:', () => {
     );
     load.stub
       .withArgs(idToPath('a'))
-      .returns({file: createFileFromId('a'), dependencies: ['b', 'c', 'd']})
+      .returns({
+        file: createFileFromId('a'),
+        dependencies: ['b', 'c', 'd'].map(depOf),
+      })
       .withArgs(idToPath('b'))
-      .returns({file: createFileFromId('b'), dependencies: ['e']});
+      .returns({file: createFileFromId('b'), dependencies: [depOf('e')]});
     ['c', 'd', 'e'].forEach(id =>
       load.stub
         .withArgs(idToPath(id))
@@ -405,7 +419,7 @@ describe('Graph:', () => {
 });
 
 function createDependency(id) {
-  return {id, path: idToPath(id)};
+  return {id, path: idToPath(id), isAsync: false};
 }
 
 function createFileFromId(id) {
@@ -427,6 +441,10 @@ function idToPath(id) {
   return '/path/to/' + id;
 }
 
+function depOf(name) {
+  return {name, isAsync: false};
+}
+
 function deferred(value) {
   let resolve;
   const promise = new Promise(res => (resolve = res));

packages/metro-bundler/src/ModuleGraph/output/__tests__/dependencies-dot.js

@@ -115,7 +115,7 @@ function createModule(path: string, deps: Array<string>) {
       path,
       type: 'module',
     },
-    dependencies: deps.map(d => ({id: d, path: d})),
+    dependencies: deps.map(d => ({id: d, path: d, isAsync: false})),
   };
 }
 

packages/metro-bundler/src/ModuleGraph/output/__tests__/indexed-ram-bundle-test.js

@@ -259,6 +259,7 @@ function makeDependency(name) {
   const path = makeModulePath(name);
   return {
     id: name,
+    isAsync: false,
     path,
   };
 }

packages/metro-bundler/src/ModuleGraph/output/__tests__/multiple-files-ram-bundle-test.js

@@ -145,6 +145,7 @@ function makeDependency(name) {
   const path = makeModulePath(name);
   return {
     id: name,
+    isAsync: false,
     path,
   };
 }

packages/metro-bundler/src/ModuleGraph/types.flow.js

@@ -26,6 +26,7 @@ export type Callback<A = void, B = void> = (Error => void) &
 
 type Dependency = {|
   id: string,
+  +isAsync: boolean,
   path: string,
 |};
 
@@ -79,7 +80,7 @@ export type IdsForPathFn = ({path: string}) => ModuleIds;
 
 export type LoadResult = {
   file: File,
-  dependencies: Array<string>,
+  dependencies: $ReadOnlyArray<TransformResultDependency>,
 };
 
 export type LoadFn = (
@@ -141,9 +142,22 @@ export type TransformerResult = {|
   map: ?MappingsMap,
 |};
 
+export type TransformResultDependency = {|
+  /**
+   * The literal name provided to a require or import call. For example 'foo' in
+   * case of `require('foo')`.
+   */
+  +name: string,
+  /**
+   * If `true` this dependency is due to a dynamic `import()` call. If `false`,
+   * this dependency was pulled using a synchronous `require()` call.
+   */
+  +isAsync: boolean,
+|};
+
 export type TransformResult = {|
   code: string,
-  dependencies: Array<string>,
+  dependencies: $ReadOnlyArray<TransformResultDependency>,
   dependencyMapName?: string,
   map: ?MappingsMap,
 |};

packages/metro-bundler/src/ModuleGraph/worker/__tests__/collect-dependencies-test.js

@@ -32,10 +32,11 @@ describe('dependency collection from ASTs', () => {
       }
     `);
 
-    expect(collectDependencies(ast).dependencies).toEqual([
-      'b/lib/a',
-      'do',
-      'setup/something',
+    const result = collectDependencies(ast);
+    expect(result.dependencies).toEqual([
+      {name: 'b/lib/a', isAsync: false},
+      {name: 'do', isAsync: false},
+      {name: 'setup/something', isAsync: false},
     ]);
   });
 
@@ -47,17 +48,33 @@ describe('dependency collection from ASTs', () => {
       }
     `);
 
-    expect(collectDependencies(ast).dependencies).toEqual([
-      'b/lib/a',
-      'some/async/module',
-      'BundleSegments',
+    const result = collectDependencies(ast);
+    expect(result.dependencies).toEqual([
+      {name: 'b/lib/a', isAsync: false},
+      {name: 'some/async/module', isAsync: true},
+      {name: 'BundleSegments', isAsync: false},
+    ]);
+  });
+
+  it('collects mixed dependencies as being sync', () => {
+    const ast = astFromCode(`
+      const a = require('b/lib/a');
+      import('b/lib/a');
+    `);
+
+    const result = collectDependencies(ast);
+    expect(result.dependencies).toEqual([
+      {name: 'b/lib/a', isAsync: false},
+      {name: 'BundleSegments', isAsync: false},
     ]);
   });
 
   it('supports template literals as arguments', () => {
     const ast = astFromCode('require(`left-pad`)');
 
-    expect(collectDependencies(ast).dependencies).toEqual(['left-pad']);
+    expect(collectDependencies(ast).dependencies).toEqual([
+      {name: 'left-pad', isAsync: false},
+    ]);
   });
 
   it('throws on template literals with interpolations', () => {
@@ -114,7 +131,7 @@ describe('dependency collection from ASTs', () => {
 describe('Dependency collection from optimized ASTs', () => {
   const dependencyMapName = 'arbitrary';
   const {forOptimization} = collectDependencies;
-  let ast, names;
+  let ast, deps;
 
   beforeEach(() => {
     ast = astFromCode(`
@@ -124,27 +141,39 @@ describe('Dependency collection from optimized ASTs', () => {
         require(${dependencyMapName}[2], "setup/something");
       }
     `);
-    names = ['b/lib/a', 'do', 'setup/something'];
+    deps = [
+      {name: 'b/lib/a', isAsync: false},
+      {name: 'do', isAsync: false},
+      {name: 'setup/something', isAsync: true},
+    ];
   });
 
   it('passes the `dependencyMapName` through', () => {
-    const result = forOptimization(ast, names, dependencyMapName);
+    const result = forOptimization(ast, deps, dependencyMapName);
     expect(result.dependencyMapName).toEqual(dependencyMapName);
   });
 
   it('returns the list of passed in dependencies', () => {
-    const result = forOptimization(ast, names, dependencyMapName);
-    expect(result.dependencies).toEqual(names);
+    const result = forOptimization(ast, deps, dependencyMapName);
+    expect(result.dependencies).toEqual(deps);
   });
 
   it('only returns dependencies that are in the code', () => {
     ast = astFromCode(`require(${dependencyMapName}[1], 'do')`);
-    const result = forOptimization(ast, names, dependencyMapName);
-    expect(result.dependencies).toEqual(['do']);
+    const result = forOptimization(ast, deps, dependencyMapName);
+    expect(result.dependencies).toEqual([{name: 'do', isAsync: false}]);
+  });
+
+  it('only returns async dependencies that are in the code', () => {
+    ast = astFromCode(`require(${dependencyMapName}[2], "setup/something")`);
+    const result = forOptimization(ast, deps, dependencyMapName);
+    expect(result.dependencies).toEqual([
+      {name: 'setup/something', isAsync: true},
+    ]);
   });
 
   it('replaces all call signatures inserted by a prior call to `collectDependencies`', () => {
-    forOptimization(ast, names, dependencyMapName);
+    forOptimization(ast, deps, dependencyMapName);
     expect(codeFromAst(ast)).toEqual(
       comparableCode(`
       const a = require(${dependencyMapName}[0]);

packages/metro-bundler/src/ModuleGraph/worker/__tests__/optimize-module-test.js

@@ -78,7 +78,9 @@ describe('optimizing JS modules', () => {
     });
 
     it('extracts dependencies', () => {
-      expect(optimized.dependencies).toEqual(['arbitrary-android-prod']);
+      expect(optimized.dependencies).toEqual([
+        {name: 'arbitrary-android-prod', isAsync: false},
+      ]);
     });
 
     it('creates source maps', () => {

packages/metro-bundler/src/ModuleGraph/worker/__tests__/transform-module-test.js

@@ -166,7 +166,12 @@ describe('transforming JS modules:', () => {
     const result = transformModule(toBuffer(code), options());
     invariant(result.type === 'code', 'result must be code');
     expect(result.details.transformed.default).toEqual(
-      expect.objectContaining({dependencies: [dep1, dep2]}),
+      expect.objectContaining({
+        dependencies: [
+          {name: dep1, isAsync: false},
+          {name: dep2, isAsync: false},
+        ],
+      }),
     );
   });
 

packages/metro-bundler/src/ModuleGraph/worker/collect-dependencies.js

@@ -18,15 +18,12 @@ const nullthrows = require('fbjs/lib/nullthrows');
 const {traverse, types} = require('babel-core');
 const prettyPrint = require('babel-generator').default;
 
-class Replacement {
-  nameToIndex: Map<string, number>;
-  nextIndex: number;
-  replaceImports = true;
+import type {TransformResultDependency} from '../types.flow';
 
-  constructor() {
-    this.nameToIndex = new Map();
-    this.nextIndex = 0;
-  }
+class Replacement {
+  nameToIndex: Map<string, number> = new Map();
+  dependencies: Array<{|+name: string, isAsync: boolean|}> = [];
+  replaceImports = true;
 
   getRequireCallArg(node) {
     const args = node.arguments;
@@ -40,21 +37,25 @@ class Replacement {
     return args[0];
   }
 
-  getIndex(stringLiteralOrTemplateLiteral) {
+  getIndex(stringLiteralOrTemplateLiteral, isAsync: boolean) {
     const name = stringLiteralOrTemplateLiteral.quasis
       ? stringLiteralOrTemplateLiteral.quasis[0].value.cooked
       : stringLiteralOrTemplateLiteral.value;
     let index = this.nameToIndex.get(name);
     if (index !== undefined) {
+      if (!isAsync) {
+        this.dependencies[index].isAsync = false;
+      }
       return index;
     }
-    index = this.nextIndex++;
+
+    index = this.dependencies.push({name, isAsync}) - 1;
     this.nameToIndex.set(name, index);
     return index;
   }
 
-  getNames() {
-    return Array.from(this.nameToIndex.keys());
+  getDependencies(): $ReadOnlyArray<TransformResultDependency> {
+    return this.dependencies;
   }
 
   makeArgs(newId, oldId, dependencyMapIdentifier) {
@@ -73,12 +74,12 @@ function getInvalidProdRequireMessage(node) {
 
 class ProdReplacement {
   replacement: Replacement;
-  names: Array<string>;
+  dependencies: $ReadOnlyArray<TransformResultDependency>;
   replaceImports = false;
 
-  constructor(names) {
+  constructor(dependencies: $ReadOnlyArray<TransformResultDependency>) {
     this.replacement = new Replacement();
-    this.names = names;
+    this.dependencies = dependencies;
   }
 
   getRequireCallArg(node) {
@@ -99,21 +100,23 @@ class ProdReplacement {
     return args[0];
   }
 
-  getIndex(memberExpression) {
+  getIndex(memberExpression, _: boolean) {
     const id = memberExpression.property.value;
-    if (id in this.names) {
-      return this.replacement.getIndex({value: this.names[id]});
+    if (id in this.dependencies) {
+      const dependency = this.dependencies[id];
+      const xp = {value: dependency.name};
+      return this.replacement.getIndex(xp, dependency.isAsync);
     }
 
     throw new Error(
-      `${id} is not a known module ID. Existing mappings: ${this.names
-        .map((n, i) => `${i} => ${n}`)
+      `${id} is not a known module ID. Existing mappings: ${this.dependencies
+        .map((n, i) => `${i} => ${n.name}`)
         .join(', ')}`,
     );
   }
 
-  getNames() {
-    return this.replacement.getNames();
+  getDependencies(): $ReadOnlyArray<TransformResultDependency> {
+    return this.replacement.getDependencies();
   }
 
   makeArgs(newId, _, dependencyMapIdentifier) {
@@ -147,7 +150,8 @@ function collectDependencies(ast, replacement, dependencyMapIdentifier) {
       CallExpression(path, state) {
         const node = path.node;
         if (replacement.replaceImports && node.callee.type === 'Import') {
-          processImportCall(path, node, replacement, state);
+          const reqNode = processImportCall(path, node, replacement, state);
+          visited.add(reqNode);
           return;
         }
         if (visited.has(node)) {
@@ -157,7 +161,7 @@ function collectDependencies(ast, replacement, dependencyMapIdentifier) {
           return;
         }
         const arg = replacement.getRequireCallArg(node);
-        const index = replacement.getIndex(arg);
+        const index = replacement.getIndex(arg, false);
         node.arguments = replacement.makeArgs(
           types.numericLiteral(index),
           arg,
@@ -171,14 +175,14 @@ function collectDependencies(ast, replacement, dependencyMapIdentifier) {
   );
 
   return {
-    dependencies: replacement.getNames(),
+    dependencies: replacement.getDependencies(),
     dependencyMapName: nullthrows(traversalState.dependencyMapIdentifier).name,
   };
 }
 
 const makeAsyncRequire = babelTemplate(
   `require(BUNDLE_SEGMENTS_PATH).loadForModule(MODULE_ID).then(
-    function() { return require(MODULE_PATH); }
+    function() { return require(REQUIRE_ARGS); }
   )`,
 );
 
@@ -192,9 +196,13 @@ function processImportCall(path, node, replacement, state) {
     );
   }
   const modulePath = args[0];
-  const index = replacement.getIndex(modulePath);
+  const index = replacement.getIndex(modulePath, true);
   const newImport = makeAsyncRequire({
-    MODULE_PATH: modulePath,
+    REQUIRE_ARGS: replacement.makeArgs(
+      types.numericLiteral(index),
+      modulePath,
+      state.dependencyMapIdentifier,
+    ),
     MODULE_ID: createMapLookup(
       state.dependencyMapIdentifier,
       types.numericLiteral(index),
@@ -205,6 +213,9 @@ function processImportCall(path, node, replacement, state) {
     },
   });
   path.replaceWith(newImport);
+  // This is the inner require() call. We return it so it
+  // gets marked as already visited.
+  return newImport.expression.arguments[0].body.body[0].argument;
 }
 
 function isLiteralString(node) {
@@ -229,12 +240,12 @@ const xp = (module.exports = (ast: Ast) =>
 
 xp.forOptimization = (
   ast: Ast,
-  names: Array<string>,
+  dependencies: $ReadOnlyArray<TransformResultDependency>,
   dependencyMapName?: string,
 ) =>
   collectDependencies(
     ast,
-    new ProdReplacement(names),
+    new ProdReplacement(dependencies),
     dependencyMapName ? types.identifier(dependencyMapName) : undefined,
   );
 

packages/metro-bundler/src/ModuleGraph/worker/transform-module.js

@@ -188,7 +188,12 @@ function makeResult(ast: Ast, filename, sourceCode, isPolyfill = false) {
   }
 
   const gen = generate(file, filename, sourceCode, false);
-  return {code: gen.code, map: gen.map, dependencies, dependencyMapName};
+  return {
+    code: gen.code,
+    map: gen.map,
+    dependencies,
+    dependencyMapName,
+  };
 }
 
 module.exports = transformModule;