From 5e9cf9ec4b359cb42577df1f0e529dbd259884a2 Mon Sep 17 00:00:00 2001 From: Rafael Oleza Date: Mon, 19 Mar 2018 10:04:41 -0700 Subject: [PATCH] Start using the Graph object in the traverse dependencies logic Reviewed By: davidaurelio Differential Revision: D7180698 fbshipit-source-id: a7c39b73a7673f450e3a147e7feff4cfcc3061bf --- .../metro/src/DeltaBundler/DeltaCalculator.js | 18 ++- .../__tests__/DeltaCalculator-test.js | 14 +-- .../traverseDependencies-test.js.snap | 109 +++++++++--------- .../traverseDependencies-integration-test.js | 17 ++- .../__tests__/traverseDependencies-test.js | 99 +++++++--------- .../src/DeltaBundler/traverseDependencies.js | 55 +++++---- 6 files changed, 154 insertions(+), 158 deletions(-) diff --git a/packages/metro/src/DeltaBundler/DeltaCalculator.js b/packages/metro/src/DeltaBundler/DeltaCalculator.js index c925f22a..052d45c8 100644 --- a/packages/metro/src/DeltaBundler/DeltaCalculator.js +++ b/packages/metro/src/DeltaBundler/DeltaCalculator.js @@ -21,7 +21,7 @@ import type Bundler from '../Bundler'; import type {Options as JSTransformerOptions} from '../JSTransformer/worker'; import type DependencyGraph from '../node-haste/DependencyGraph'; import type {BundleOptions} from '../shared/types.flow'; -import type {DependencyEdge, DependencyEdges} from './traverseDependencies'; +import type {DependencyEdge, Graph} from './traverseDependencies'; export type DeltaResult = {| +modified: Map, @@ -29,10 +29,7 @@ export type DeltaResult = {| +reset: boolean, |}; -export type Graph = {| - dependencies: DependencyEdges, - entryFile: string, -|}; +export type {Graph} from './traverseDependencies'; /** * This class is in charge of calculating the delta of changed modules that @@ -203,10 +200,12 @@ class DeltaCalculator extends EventEmitter { {dev: this._options.dev, platform: this._options.platform}, async path => { const {added} = await initialTraverseDependencies( - path, + { + dependencies: new Map(), + entryFile: path, + }, this._dependencyGraph, transformOptionsForBlacklist, - new Map(), ); return Array.from(added.keys()); @@ -266,10 +265,9 @@ class DeltaCalculator extends EventEmitter { if (!this._graph.dependencies.size) { const {added} = await initialTraverseDependencies( - this._graph.entryFile, + this._graph, this._dependencyGraph, transformerOptions, - this._graph.dependencies, this._options.onProgress || undefined, ); @@ -304,7 +302,7 @@ class DeltaCalculator extends EventEmitter { modifiedDependencies, this._dependencyGraph, transformerOptions, - this._graph.dependencies, + this._graph, this._options.onProgress || undefined, ); diff --git a/packages/metro/src/DeltaBundler/__tests__/DeltaCalculator-test.js b/packages/metro/src/DeltaBundler/__tests__/DeltaCalculator-test.js index 49bdeddd..604a48cc 100644 --- a/packages/metro/src/DeltaBundler/__tests__/DeltaCalculator-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/DeltaCalculator-test.js @@ -91,7 +91,7 @@ describe('DeltaCalculator', () => { }; initialTraverseDependencies.mockImplementationOnce( - async (path, dg, opt, edges) => { + async (graph, dg, opt) => { edgeModule = { ...entryModule, dependencies: new Map([ @@ -116,10 +116,10 @@ describe('DeltaCalculator', () => { inverseDependencies: ['/bundle'], }; - edges.set('/bundle', edgeModule); - edges.set('/foo', edgeFoo); - edges.set('/bar', edgeBar); - edges.set('/baz', edgeBaz); + graph.dependencies.set('/bundle', edgeModule); + graph.dependencies.set('/foo', edgeFoo); + graph.dependencies.set('/bar', edgeBar); + graph.dependencies.set('/baz', edgeBaz); return { added: new Map([ @@ -274,8 +274,8 @@ describe('DeltaCalculator', () => { mockedDependencies.push(moduleQux); - traverseDependencies.mockImplementation(async (path, dg, opt, edges) => { - edges.set('/qux', edgeQux); + traverseDependencies.mockImplementation(async (path, dg, opt, graph) => { + graph.dependencies.set('/qux', edgeQux); return { added: new Map([['/foo', edgeFoo], ['/qux', edgeQux]]), diff --git a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap index 9356947c..b726d4b2 100644 --- a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap @@ -1,61 +1,64 @@ // Jest Snapshot v1, https://goo.gl/fbAQLP exports[`should do the initial traversal correctly 1`] = ` -Map { - "/bundle" => Object { - "dependencies": Map { - "foo" => "/foo", +Object { + "dependencies": Map { + "/bundle" => Object { + "dependencies": Map { + "foo" => "/foo", + }, + "inverseDependencies": Set {}, + "output": Object { + "code": "// code", + "map": Array [], + "source": "// source", + "type": "module", + }, + "path": "/bundle", }, - "inverseDependencies": Set {}, - "output": Object { - "code": "// code", - "map": Array [], - "source": "// source", - "type": "module", + "/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", }, - "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", }, + "entryFile": "/bundle", } `; diff --git a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-integration-test.js b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-integration-test.js index 7f2fa1e4..7f4901b8 100644 --- a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-integration-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-integration-test.js @@ -60,17 +60,20 @@ describe('traverseDependencies', function() { ) { const dgraph = await dgraphPromise; - const edges = new Map(); + const graph = { + dependencies: new Map(), + entryFile: entryPath, + }; + const {added} = await traverseDependencies.initialTraverseDependencies( - entryPath, + graph, dgraph, {...emptyTransformOptions, platform}, - edges, ); const dependencies = recursive ? [...added.values()].map(edge => edge.path) - : edges.get(entryPath).dependencies.values(); + : graph.dependencies.get(entryPath).dependencies.values(); return await Promise.all( [...dependencies].map(async path => { @@ -5068,10 +5071,12 @@ describe('traverseDependencies', function() { function getDependencies() { return traverseDependencies.initialTraverseDependencies( - '/root/index.js', + { + dependencies: new Map(), + entryFile: '/root/index.js', + }, dependencyGraph, emptyTransformOptions, - new Map(), onProgress, ); } diff --git a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js index d2484e23..646cac47 100644 --- a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js @@ -19,6 +19,7 @@ let dependencyGraph; let mockedDependencies; let mockedDependencyTree; let files = new Set(); +let graph; let entryModule; let moduleFoo; @@ -168,32 +169,30 @@ beforeEach(async () => { Actions.addDependency('/foo', '/baz'); files = new Set(); + + graph = { + dependencies: new Map(), + entryFile: '/bundle', + }; }); it('should do the initial traversal correctly', async () => { - const edges = new Map(); - const result = await initialTraverseDependencies( - '/bundle', - dependencyGraph, - {}, - edges, - ); + const result = await initialTraverseDependencies(graph, dependencyGraph, {}); expect(getPaths(result)).toEqual({ added: new Set(['/bundle', '/foo', '/bar', '/baz']), deleted: new Set(), }); - expect(edges).toMatchSnapshot(); + expect(graph).toMatchSnapshot(); }); it('should return an empty result when there are no changes', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); expect( getPaths( - await traverseDependencies(['/bundle'], dependencyGraph, {}, edges), + await traverseDependencies(['/bundle'], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/bundle']), @@ -202,14 +201,13 @@ it('should return an empty result when there are no changes', async () => { }); it('should return a removed dependency', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.removeDependency('/foo', '/bar'); expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/foo']), @@ -218,8 +216,7 @@ it('should return a removed dependency', async () => { }); it('should return added/removed dependencies', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.addDependency('/foo', '/qux'); Actions.removeDependency('/foo', '/bar'); @@ -227,7 +224,7 @@ it('should return added/removed dependencies', async () => { expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/foo', '/qux']), @@ -236,8 +233,7 @@ it('should return added/removed dependencies', async () => { }); it('should return added modules before the modified ones', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.addDependency('/foo', '/qux'); Actions.modifyFile('/bar'); @@ -247,32 +243,30 @@ it('should return added modules before the modified ones', async () => { // it to an array. expect([ ...getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ).added, ]).toEqual(['/qux', '/foo', '/bar', '/baz']); }); it('should retry to traverse the dependencies as it was after getting an error', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.deleteFile(moduleBar.path); await expect( - traverseDependencies(['/foo'], dependencyGraph, {}, edges), + traverseDependencies(['/foo'], dependencyGraph, {}, graph), ).rejects.toBeInstanceOf(Error); // Second time that the traversal of dependencies we still have to throw an // error (no matter if no file has been changed). await expect( - traverseDependencies(['/foo'], dependencyGraph, {}, edges), + traverseDependencies(['/foo'], dependencyGraph, {}, graph), ).rejects.toBeInstanceOf(Error); }); describe('edge cases', () => { it('should handle renames correctly', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.removeDependency('/foo', '/baz'); Actions.moveFile('/baz', '/qux'); @@ -280,7 +274,7 @@ describe('edge cases', () => { expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/foo', '/qux']), @@ -289,8 +283,7 @@ describe('edge cases', () => { }); it('should not try to remove wrong dependencies when renaming files', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); // Rename /foo to /foo-renamed, but keeping all its dependencies. Actions.addDependency('/bundle', '/foo-renamed'); @@ -302,7 +295,7 @@ describe('edge cases', () => { expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/bundle', '/foo-renamed']), @@ -311,15 +304,14 @@ describe('edge cases', () => { }); it('modify a file and delete it afterwards', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.modifyFile('/baz'); Actions.removeDependency('/foo', '/baz'); expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/foo']), @@ -328,15 +320,14 @@ describe('edge cases', () => { }); it('move a file to a different folder', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.addDependency('/foo', '/baz-moved'); Actions.removeDependency('/foo', '/baz'); expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/foo', '/baz-moved']), @@ -345,21 +336,20 @@ describe('edge cases', () => { }); it('maintain the order of module dependencies consistent', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); Actions.addDependency('/foo', '/qux', 0); expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/foo', '/qux']), deleted: new Set(), }); - expect([...edges.get(moduleFoo.path).dependencies]).toEqual([ + expect([...graph.dependencies.get(moduleFoo.path).dependencies]).toEqual([ ['qux', '/qux'], ['bar', '/bar'], ['baz', '/baz'], @@ -367,22 +357,21 @@ describe('edge cases', () => { }); it('should create two entries when requiring the same file in different forms', async () => { - const edges = new Map(); - await initialTraverseDependencies('/bundle', dependencyGraph, {}, edges); + await initialTraverseDependencies(graph, dependencyGraph, {}); // We're adding a new reference from bundle to foo. Actions.addDependency('/bundle', '/foo', 0, 'foo.js'); expect( getPaths( - await traverseDependencies([...files], dependencyGraph, {}, edges), + await traverseDependencies([...files], dependencyGraph, {}, graph), ), ).toEqual({ added: new Set(['/bundle']), deleted: new Set(), }); - expect([...edges.get(entryModule.path).dependencies]).toEqual([ + expect([...graph.dependencies.get(entryModule.path).dependencies]).toEqual([ ['foo.js', '/foo'], ['foo', '/foo'], ]); @@ -425,15 +414,15 @@ describe('edge cases', () => { } async function assertOrder() { + graph = { + dependencies: new Map(), + entryFile: '/bundle', + }; + expect( Array.from( getPaths( - await initialTraverseDependencies( - '/bundle', - dependencyGraph, - {}, - new Map(), - ), + await initialTraverseDependencies(graph, dependencyGraph, {}), ).added, ), ).toEqual(['/bundle', '/foo', '/baz', '/bar']); @@ -467,7 +456,6 @@ describe('edge cases', () => { jest.spyOn(moduleBar, 'read'); jest.spyOn(moduleBaz, 'read'); - const edges = new Map(); const transformOptions = { inlineRequires: { blacklist: { @@ -476,12 +464,7 @@ describe('edge cases', () => { }, }; - await initialTraverseDependencies( - '/bundle', - dependencyGraph, - transformOptions, - edges, - ); + await initialTraverseDependencies(graph, dependencyGraph, transformOptions); expect(entryModule.read).toHaveBeenCalledWith({inlineRequires: true}); expect(moduleFoo.read).toHaveBeenCalledWith({inlineRequires: true}); @@ -494,7 +477,7 @@ describe('edge cases', () => { ['/foo'], dependencyGraph, transformOptions, - edges, + graph, ); expect(moduleFoo.read).toHaveBeenCalledWith({inlineRequires: true}); diff --git a/packages/metro/src/DeltaBundler/traverseDependencies.js b/packages/metro/src/DeltaBundler/traverseDependencies.js index df8a70c9..6057378f 100644 --- a/packages/metro/src/DeltaBundler/traverseDependencies.js +++ b/packages/metro/src/DeltaBundler/traverseDependencies.js @@ -33,6 +33,11 @@ export type DependencyEdge = {| export type DependencyEdges = Map; +export type Graph = {| + dependencies: DependencyEdges, + entryFile: string, +|}; + type Result = {added: Map, deleted: Set}; /** @@ -65,7 +70,7 @@ async function traverseDependencies( paths: Array, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, - edges: DependencyEdges, + graph: Graph, onProgress?: (numProcessed: number, total: number) => mixed = () => {}, ): Promise { const delta = { @@ -76,7 +81,7 @@ async function traverseDependencies( await Promise.all( paths.map(async path => { - const edge = edges.get(path); + const edge = graph.dependencies.get(path); if (!edge) { return; @@ -88,7 +93,7 @@ async function traverseDependencies( edge, dependencyGraph, transformOptions, - edges, + graph, delta, onProgress, ); @@ -129,13 +134,15 @@ async function traverseDependencies( } async function initialTraverseDependencies( - path: string, + graph: Graph, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, - edges: DependencyEdges, onProgress?: (numProcessed: number, total: number) => mixed = () => {}, ): Promise { - const edge = createEdge(dependencyGraph.getModuleForPath(path), edges); + const edge = createEdge( + dependencyGraph.getModuleForPath(graph.entryFile), + graph, + ); const delta = { added: new Map([[edge.path, edge]]), @@ -147,7 +154,7 @@ async function initialTraverseDependencies( edge, dependencyGraph, transformOptions, - edges, + graph, delta, onProgress, ); @@ -162,7 +169,7 @@ async function traverseDependenciesForSingleFile( edge: DependencyEdge, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, - edges: DependencyEdges, + graph: Graph, delta: Delta, onProgress?: (numProcessed: number, total: number) => mixed = () => {}, ): Promise { @@ -174,7 +181,7 @@ async function traverseDependenciesForSingleFile( edge, dependencyGraph, transformOptions, - edges, + graph, delta, () => { total++; @@ -194,7 +201,7 @@ async function processEdge( edge: DependencyEdge, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, - edges: DependencyEdges, + graph: Graph, delta: Delta, onDependencyAdd: () => mixed, onDependencyAdded: () => mixed, @@ -228,7 +235,7 @@ async function processEdge( for (const [relativePath, absolutePath] of previousDependencies) { if (!currentDependencies.has(relativePath)) { - removeDependency(edge, absolutePath, edges, delta); + removeDependency(edge, absolutePath, graph, delta); } } @@ -247,7 +254,7 @@ async function processEdge( absolutePath, dependencyGraph, transformOptions, - edges, + graph, delta, onDependencyAdd, onDependencyAdded, @@ -262,12 +269,12 @@ async function addDependency( path: string, dependencyGraph: DependencyGraph, transformOptions: JSTransformerOptions, - edges: DependencyEdges, + graph: Graph, delta: Delta, onDependencyAdd: () => mixed, onDependencyAdded: () => mixed, ): Promise { - const existingEdge = edges.get(path); + const existingEdge = graph.dependencies.get(path); // The new dependency was already in the graph, we don't need to do anything. if (existingEdge) { @@ -276,7 +283,7 @@ async function addDependency( return; } - const edge = createEdge(dependencyGraph.getModuleForPath(path), edges); + const edge = createEdge(dependencyGraph.getModuleForPath(path), graph); edge.inverseDependencies.add(parentEdge.path); delta.added.set(edge.path, edge); @@ -287,7 +294,7 @@ async function addDependency( edge, dependencyGraph, transformOptions, - edges, + graph, delta, onDependencyAdd, onDependencyAdded, @@ -299,10 +306,10 @@ async function addDependency( function removeDependency( parentEdge: DependencyEdge, absolutePath: string, - edges: DependencyEdges, + graph: Graph, delta: Delta, ): void { - const edge = edges.get(absolutePath); + const edge = graph.dependencies.get(absolutePath); if (!edge) { return; @@ -322,14 +329,14 @@ function removeDependency( // clean up everything (we cannot read the module because it may have // been deleted). for (const depAbsolutePath of edge.dependencies.values()) { - removeDependency(edge, depAbsolutePath, edges, delta); + removeDependency(edge, depAbsolutePath, graph, delta); } // This module is not used anywhere else!! we can clear it from the bundle - destroyEdge(edge, edges); + destroyEdge(edge, graph); } -function createEdge(module: Module, edges: DependencyEdges): DependencyEdge { +function createEdge(module: Module, graph: Graph): DependencyEdge { const edge = { dependencies: new Map(), inverseDependencies: new Set(), @@ -341,7 +348,7 @@ function createEdge(module: Module, edges: DependencyEdges): DependencyEdge { type: getType(module), }, }; - edges.set(module.path, edge); + graph.dependencies.set(module.path, edge); return edge; } @@ -358,8 +365,8 @@ function getType(module: Module): DependencyType { return 'module'; } -function destroyEdge(edge: DependencyEdge, edges: DependencyEdges) { - edges.delete(edge.path); +function destroyEdge(edge: DependencyEdge, graph: Graph) { + graph.dependencies.delete(edge.path); } function resolveDependencies(