From 25b201148b31bd15d520ad668232ad6e577be435 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Mon, 19 Feb 2018 06:41:50 -0800 Subject: [PATCH] Refactor the traversal of dependencies logic Reviewed By: jeanlauliac Differential Revision: D7009928 fbshipit-source-id: a56e4d3b5a3b2c91f14affbc56cbca56c8526d06 --- ...verseDependencies-integration-test.js.snap | 4 +- .../traverseDependencies-test.js.snap | 61 ++++ .../__tests__/traverseDependencies-test.js | 30 +- .../src/DeltaBundler/traverseDependencies.js | 304 ++++++++---------- 4 files changed, 228 insertions(+), 171 deletions(-) create mode 100644 packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap diff --git a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-integration-test.js.snap b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-integration-test.js.snap index a2910b84..fd31cafd 100644 --- a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-integration-test.js.snap +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-integration-test.js.snap @@ -39,8 +39,8 @@ Array [ 7, ], Array [ - 3, - 7, + 2, + 8, ], Array [ 3, diff --git a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap new file mode 100644 index 00000000..9356947c --- /dev/null +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap @@ -0,0 +1,61 @@ +// Jest Snapshot v1, https://goo.gl/fbAQLP + +exports[`should do the initial traversal correctly 1`] = ` +Map { + "/bundle" => Object { + "dependencies": Map { + "foo" => "/foo", + }, + "inverseDependencies": Set {}, + "output": Object { + "code": "// code", + "map": Array [], + "source": "// source", + "type": "module", + }, + "path": "/bundle", + }, + "/foo" => Object { + "dependencies": Map { + "bar" => "/bar", + "baz" => "/baz", + }, + "inverseDependencies": Set { + "/bundle", + }, + "output": Object { + "code": "// code", + "map": Array [], + "source": "// source", + "type": "module", + }, + "path": "/foo", + }, + "/bar" => Object { + "dependencies": Map {}, + "inverseDependencies": Set { + "/foo", + }, + "output": Object { + "code": "// code", + "map": Array [], + "source": "// source", + "type": "module", + }, + "path": "/bar", + }, + "/baz" => Object { + "dependencies": Map {}, + "inverseDependencies": Set { + "/foo", + }, + "output": Object { + "code": "// code", + "map": Array [], + "source": "// source", + "type": "module", + }, + "path": "/baz", + }, +} +`; diff --git a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js index b3b602bc..8f98a588 100644 --- a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js @@ -83,7 +83,9 @@ beforeEach(async () => { const dependency = deps.filter(dep => dep.name === relativePath)[0]; if (!mockedDependencies.has(dependency)) { - throw new Error('Dependency not found'); + throw new Error( + `Dependency not found: ${module.path}->${relativePath}`, + ); } return dependency; }, @@ -103,6 +105,8 @@ it('should do the initial traversal correctly', async () => { added: new Set(['/bundle', '/foo', '/bar', '/baz']), deleted: new Set(), }); + + expect(edges).toMatchSnapshot(); }); it('should return an empty result when there are no changes', async () => { @@ -241,6 +245,30 @@ describe('edge cases', () => { }); }); + it('maintain the order of module dependencies consistent', async () => { + const edges = new Map(); + await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + + const moduleQux = createModule({path: '/qux', name: 'qux'}); + mockedDependencyTree.set(moduleFoo.path, [moduleQux, moduleBar, moduleBaz]); + mockedDependencies.add(moduleQux); + + expect( + getPaths( + await traverseDependencies(['/foo'], dependencyGraph, {}, edges), + ), + ).toEqual({ + added: new Set(['/foo', '/qux']), + deleted: new Set(), + }); + + expect([...edges.get(moduleFoo.path).dependencies]).toEqual([ + ['qux', moduleQux.path], + ['bar', moduleBar.path], + ['baz', moduleBaz.path], + ]); + }); + it('should traverse the dependency tree in a deterministic order', async () => { // Mocks the shallow dependency call, always resolving the module in // `slowPath` after the module in `fastPath`. diff --git a/packages/metro/src/DeltaBundler/traverseDependencies.js b/packages/metro/src/DeltaBundler/traverseDependencies.js index e277a5ca..be27ad97 100644 --- a/packages/metro/src/DeltaBundler/traverseDependencies.js +++ b/packages/metro/src/DeltaBundler/traverseDependencies.js @@ -56,16 +56,26 @@ async function traverseDependencies( edges: DependencyEdges, onProgress?: (numProcessed: number, total: number) => mixed = () => {}, ): Promise { - const changes = await Promise.all( - paths.map(path => - traverseDependenciesForSingleFile( - path, - dependencyGraph, - transformOptions, - edges, - onProgress, - ), - ), + const changes = []; + + await Promise.all( + paths.map(async path => { + const edge = edges.get(path); + + if (!edge) { + return; + } + + changes.push( + await traverseDependenciesForSingleFile( + edge, + dependencyGraph, + transformOptions, + edges, + onProgress, + ), + ); + }), ); const added = new Map(); @@ -75,6 +85,7 @@ async function traverseDependencies( for (const [path, edge] of change.added) { added.set(path, edge); } + for (const path of change.deleted) { // If a path has been marked both as added and deleted, it means that this // path is a dependency of a renamed file (or that dependency has been @@ -101,10 +112,10 @@ async function initialTraverseDependencies( edges: DependencyEdges, onProgress?: (numProcessed: number, total: number) => mixed = () => {}, ): Promise { - createEdge(dependencyGraph.getModuleForPath(path), edges); + const edge = createEdge(dependencyGraph.getModuleForPath(path), edges); return await traverseDependenciesForSingleFile( - path, + edge, dependencyGraph, transformOptions, edges, @@ -113,114 +124,125 @@ async function initialTraverseDependencies( } async function traverseDependenciesForSingleFile( - path: string, + edge: DependencyEdge, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, edges: DependencyEdges, onProgress?: (numProcessed: number, total: number) => mixed = () => {}, ): Promise { - const edge = edges.get(path); - - // If the passed edge does not exist does not exist in the graph, ignore it. - if (!edge) { - return {added: new Map(), deleted: new Set()}; - } - - const result = await dependencyGraph - .getModuleForPath(path) - .read(removeInlineRequiresBlacklistFromOptions(path, transformOptions)); - - edge.output.code = result.code; - edge.output.map = result.map; - edge.output.source = result.source; - - const shallow = result.dependencies; - - // Get the absolute path of all sub-dependencies (some of them could have been - // moved but maintain the same relative path). - const currentDependencies = resolveDependencies( - path, - shallow, - dependencyGraph, - transformOptions, - ); - - const previousDependencies = new Set(edge.dependencies.values()); - let numProcessed = 0; let total = 1; onProgress(numProcessed, total); - const deleted = Array.from(edge.dependencies.entries()) - .map(([relativePath, absolutePath]) => { - if (!currentDependencies.has(absolutePath)) { - return removeDependency(edge, relativePath, edges); - } else { - return undefined; - } - }) - .filter(Boolean); - - // Check all the module dependencies and start traversing the tree from each - // added and removed dependency, to get all the modules that have to be added - // and removed from the dependency graph. - const addedDependencies = await Promise.all( - Array.from(currentDependencies).map( - async ([absolutePath, relativePath]) => { - if (previousDependencies.has(absolutePath)) { - return new Map(); - } - - return await addDependency( - edge, - relativePath, - dependencyGraph, - transformOptions, - edges, - () => { - total++; - onProgress(numProcessed, total); - }, - () => { - numProcessed++; - onProgress(numProcessed, total); - }, - ); - }, - ), + const result = await processEdge( + edge, + dependencyGraph, + transformOptions, + edges, + () => { + total++; + onProgress(numProcessed, total); + }, + () => { + numProcessed++; + onProgress(numProcessed, total); + }, ); - const added = [new Map([[edge.path, edge]])].concat(addedDependencies); - numProcessed++; onProgress(numProcessed, total); return { - added: flattenMap(reorderDependencies(added, edges)), + added: reorderDependencies(edge.path, result.added, edges), + deleted: result.deleted, + }; +} + +async function processEdge( + edge: DependencyEdge, + dependencyGraph: DependencyGraph, + transformOptions: JSTransformerOptions, + edges: DependencyEdges, + onDependencyAdd: () => mixed, + onDependencyAdded: () => mixed, +): Promise { + const previousDependencies = new Set(edge.dependencies.values()); + + const result = await dependencyGraph + .getModuleForPath(edge.path) + .read( + removeInlineRequiresBlacklistFromOptions(edge.path, transformOptions), + ); + + // Get the absolute path of all sub-dependencies (some of them could have been + // moved but maintain the same relative path). + const currentDependencies = resolveDependencies( + edge.path, + result.dependencies, + dependencyGraph, + transformOptions, + ); + + // Update the edge information. + edge.output.code = result.code; + edge.output.map = result.map; + edge.output.source = result.source; + edge.dependencies = new Map(); + + currentDependencies.forEach((relativePath, absolutePath) => { + edge.dependencies.set(relativePath, absolutePath); + }); + + const deleted = []; + for (const absolutePath of previousDependencies.values()) { + if (!currentDependencies.has(absolutePath)) { + deleted.push(removeDependency(edge, absolutePath, edges)); + } + } + + const added = new Map([[edge.path, edge]]); + + // Check all the module dependencies and start traversing the tree from each + // added and removed dependency, to get all the modules that have to be added + // and removed from the dependency graph. + await Promise.all( + Array.from(currentDependencies.keys()).map(async absolutePath => { + if (previousDependencies.has(absolutePath)) { + return; + } + + const dependencies = await addDependency( + edge, + absolutePath, + dependencyGraph, + transformOptions, + edges, + onDependencyAdd, + onDependencyAdded, + ); + + for (const [path, edge] of dependencies) { + added.set(path, edge); + } + }), + ); + + return { + added, deleted: flatten(deleted), }; } async function addDependency( parentEdge: DependencyEdge, - relativePath: string, + path: string, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, edges: DependencyEdges, onDependencyAdd: () => mixed, onDependencyAdded: () => mixed, ): Promise> { - const parentModule = dependencyGraph.getModuleForPath(parentEdge.path); - const module = dependencyGraph.resolveDependency( - parentModule, - relativePath, - transformOptions.platform, - ); - - // Update the parent edge to keep track of the new dependency. - parentEdge.dependencies.set(relativePath, module.path); - - const existingEdge = edges.get(module.path); + const existingEdge = edges.get(path); // The new dependency was already in the graph, we don't need to do anything. if (existingEdge) { @@ -229,75 +251,52 @@ async function addDependency( return new Map(); } - onDependencyAdd(); + const edge = createEdge(dependencyGraph.getModuleForPath(path), edges); - // Create the new edge and traverse all its subdependencies, looking for new - // subdependencies recursively. - const edge = createEdge(module, edges); edge.inverseDependencies.add(parentEdge.path); - const addedDependencies = new Map([[edge.path, edge]]); + onDependencyAdd(); - const result = await module.read( - removeInlineRequiresBlacklistFromOptions(edge.path, transformOptions), + const {added} = await processEdge( + edge, + dependencyGraph, + transformOptions, + edges, + onDependencyAdd, + onDependencyAdded, ); - edge.output.code = result.code; - edge.output.map = result.map; - edge.output.source = result.source; - - const added = await Promise.all( - result.dependencies.map(dep => - addDependency( - edge, - dep, - dependencyGraph, - transformOptions, - edges, - onDependencyAdd, - onDependencyAdded, - ), - ), - ); - - for (const [newDepPath, newDepEdge] of flattenMap(added)) { - addedDependencies.set(newDepPath, newDepEdge); - } - onDependencyAdded(); - return addedDependencies; + return added; } function removeDependency( parentEdge: DependencyEdge, - relativePath: string, + absolutePath: string, edges: DependencyEdges, ): Set { - // Find the actual edge that represents the removed dependency. We do this - // from the egdes data structure, since the file may have been deleted - // already. - const dependencyEdge = resolveEdge(parentEdge, relativePath, edges); - if (!dependencyEdge) { + const edge = edges.get(absolutePath); + + if (!edge) { return new Set(); } - parentEdge.dependencies.delete(relativePath); - dependencyEdge.inverseDependencies.delete(parentEdge.path); + edge.inverseDependencies.delete(parentEdge.path); // This module is still used by another modules, so we cannot remove it from // the bundle. - if (dependencyEdge.inverseDependencies.size) { + if (edge.inverseDependencies.size) { return new Set(); } - const removedDependencies = new Set([dependencyEdge.path]); + const removedDependencies = new Set([edge.path]); // Now we need to iterate through the module dependencies in order to // clean up everything (we cannot read the module because it may have // been deleted). - for (const subDependency of dependencyEdge.dependencies.keys()) { - const removed = removeDependency(dependencyEdge, subDependency, edges); + for (const depAbsolutePath of edge.dependencies.values()) { + const removed = removeDependency(edge, depAbsolutePath, edges); for (const removedDependency of removed.values()) { removedDependencies.add(removedDependency); @@ -305,7 +304,7 @@ function removeDependency( } // This module is not used anywhere else!! we can clear it from the bundle - destroyEdge(dependencyEdge, edges); + destroyEdge(edge, edges); return removedDependencies; } @@ -343,19 +342,6 @@ function destroyEdge(edge: DependencyEdge, edges: DependencyEdges) { edges.delete(edge.path); } -function resolveEdge( - parentEdge: DependencyEdge, - relativePath: string, - edges: DependencyEdges, -): ?DependencyEdge { - const absolutePath = parentEdge.dependencies.get(relativePath); - if (!absolutePath) { - return null; - } - - return edges.get(absolutePath); -} - function resolveDependencies( parentPath, dependencies: Array, @@ -381,24 +367,6 @@ function resolveDependencies( * guarantee the same order between runs. */ function reorderDependencies( - dependencies: Array>, - edges: DependencyEdges, -): Array> { - const flatDependencies = flattenMap(dependencies); - - return dependencies.map(dependencies => { - if (dependencies.size === 0) { - return new Map(); - } - return reorderDependency( - Array.from(dependencies)[0][0], - flatDependencies, - edges, - ); - }); -} - -function reorderDependency( path: string, dependencies: Map, edges: DependencyEdges, @@ -413,7 +381,7 @@ function reorderDependency( orderedDependencies.set(path, edge); edge.dependencies.forEach(path => - reorderDependency(path, dependencies, edges, orderedDependencies), + reorderDependencies(path, dependencies, edges, orderedDependencies), ); return orderedDependencies;