From af79fbe5465a7925964af41c273255a39f851210 Mon Sep 17 00:00:00 2001 From: Moti Zilberman Date: Fri, 17 Jun 2022 07:47:46 -0700 Subject: [PATCH] Use explicit key for tracking dependencies in bundler Summary: Changes `collectDependencies` so it is responsible for providing a key with each extracted dependency. This key should be *stable* across builds and when dependencies are reordered, and *must* be locally unique within a given module. NOTE: This is a similar concept to the [`key`](https://reactjs.org/docs/lists-and-keys.html#keys) prop in React. It's also used for a similar purpose - `traverseDependencies` is not unlike a tree diffing algorithm. Previously, the `name` property ( = the first argument to `require` etc) *implicitly* served as this key in both `collectDependencies` and DeltaBundler, but since the addition of `require.context` dependency descriptors in https://github.com/facebook/metro/pull/821, this is no longer strictly correct. (This diff therefore unblocks implementing `require.context` in DeltaBundler.) Instead of teaching DeltaBundler to internally re-derive suitable keys from the dependency descriptors - essentially duplicating the logic in `collectDependencies` - the approach taken here is to require `collectDependencies` to return an *explicit* key as part of each descriptor. NOTE: Keys should be considered completely opaque. As an implementation detail (that may change without notice), we Base64-encode the keys to obfuscate them and deter downstream code from depending on the information in them. (We do this on the assumption that Base64 encoding performs better than hashing.) Note that it's safe for multiple descriptors to resolve to a single module (as of D37194640 (https://github.com/facebook/metro/commit/fc29a1177f883144674cf85a813b58567f69d545)), so from DeltaBundler's perspective it's now perfectly valid for `collectDependencies` to not collapse dependencies at all (e.g. even generate one descriptor per `require` call site) as long as it provides a unique key with each one. WARNING: This diff exposes a **preexisting** bug affecting optional dependencies (introduced in https://github.com/facebook/metro/pull/511) - see FIXME comment in `graphOperations.js` for the details. This will require more followup in a separate diff. Differential Revision: D37205825 fbshipit-source-id: 3559ee6a2f3c30017812ff009f0fe4c442e30029 --- .../helpers/__tests__/bytecode-test.js | 4 +- .../Serializers/helpers/__tests__/js-test.js | 4 +- .../traverseDependencies-test.js.snap | 12 ++- .../__tests__/traverseDependencies-test.js | 94 ++++++++++--------- .../metro/src/DeltaBundler/graphOperations.js | 89 ++++++++---------- packages/metro/src/DeltaBundler/types.flow.js | 7 +- .../__tests__/collectDependencies-test.js | 3 + .../ModuleGraph/worker/collectDependencies.js | 3 + 8 files changed, 114 insertions(+), 102 deletions(-) diff --git a/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/bytecode-test.js b/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/bytecode-test.js index 78bd566ea6..1f28402569 100644 --- a/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/bytecode-test.js +++ b/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/bytecode-test.js @@ -33,14 +33,14 @@ beforeEach(() => { 'bar', { absolutePath: '/bar', - data: {data: {asyncType: null, locs: []}, name: 'bar'}, + data: {data: {asyncType: null, locs: [], key: 'bar'}, name: 'bar'}, }, ], [ 'baz', { absolutePath: '/baz', - data: {data: {asyncType: null, locs: []}, name: 'baz'}, + data: {data: {asyncType: null, locs: [], key: 'baz'}, name: 'baz'}, }, ], ]), diff --git a/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/js-test.js b/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/js-test.js index 449d4ecb13..ad6a10eb18 100644 --- a/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/js-test.js +++ b/packages/metro/src/DeltaBundler/Serializers/helpers/__tests__/js-test.js @@ -26,14 +26,14 @@ beforeEach(() => { 'bar', { absolutePath: '/bar', - data: {data: {asyncType: null, locs: []}, name: 'bar'}, + data: {data: {asyncType: null, locs: [], key: 'bar'}, name: 'bar'}, }, ], [ 'baz', { absolutePath: '/baz', - data: {data: {asyncType: null, locs: []}, name: 'baz'}, + data: {data: {asyncType: null, locs: [], key: 'baz'}, name: 'baz'}, }, ], ]), 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 7d736b3773..b16bca6ac8 100644 --- a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/traverseDependencies-test.js.snap @@ -5,11 +5,12 @@ Object { "dependencies": Map { "/bundle" => Object { "dependencies": Map { - "foo" => Object { + "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" => Object { "absolutePath": "/foo", "data": Object { "data": Object { "asyncType": null, + "key": "C+7Hteo/D9vJXQ3UfzxbwnXaijM=", "locs": Array [], }, "name": "foo", @@ -32,21 +33,23 @@ Object { }, "/foo" => Object { "dependencies": Map { - "bar" => Object { + "Ys23Ag/5IOWqZCw9QGaVDdHwH00=" => Object { "absolutePath": "/bar", "data": Object { "data": Object { "asyncType": null, + "key": "Ys23Ag/5IOWqZCw9QGaVDdHwH00=", "locs": Array [], }, "name": "bar", }, }, - "baz" => Object { + "u+lgol6jEdIdQGaek98gA7qbkKI=" => Object { "absolutePath": "/baz", "data": Object { "data": Object { "asyncType": null, + "key": "u+lgol6jEdIdQGaek98gA7qbkKI=", "locs": Array [], }, "name": "baz", @@ -127,11 +130,12 @@ Object { "dependencies": Map { "/bundle" => Object { "dependencies": Map { - "foo" => Object { + "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" => Object { "absolutePath": "/foo", "data": Object { "data": Object { "asyncType": null, + "key": "C+7Hteo/D9vJXQ3UfzxbwnXaijM=", "locs": Array [], }, "name": "foo", diff --git a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js index 30a5793db8..c1066f5235 100644 --- a/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/traverseDependencies-test.js @@ -21,6 +21,8 @@ const { traverseDependencies: traverseDependenciesImpl, } = require('../graphOperations'); +const {objectContaining} = expect; + type DependencyDataInput = $Shape; let mockedDependencies: Set = new Set(); @@ -30,7 +32,7 @@ let mockedDependencyTree: Map< $ReadOnly<{ name: string, path: string, - data?: DependencyDataInput, + data: DependencyDataInput, }>, >, > = new Map(); @@ -79,11 +81,22 @@ const Actions = { data?: DependencyDataInput, ) { const deps = nullthrows(mockedDependencyTree.get(path)); + const depName = name ?? dependencyPath.replace('/', ''); + const key = require('crypto') + .createHash('sha1') + .update(depName) + .digest('base64'); const dep = { - name: name ?? dependencyPath.replace('/', ''), + name: depName, path: dependencyPath, - data: data ?? {}, + data: {key, ...(data ?? {})}, }; + if ( + deps.findIndex(existingDep => existingDep.data.key === dep.data.key) !== + -1 + ) { + throw new Error('Found existing mock dep with key: ' + dep.data.key); + } if (position == null) { deps.push(dep); } else { @@ -221,6 +234,7 @@ beforeEach(async () => { data: { asyncType: null, locs: [], + key: dep.data.key, ...dep.data, }, })), @@ -787,24 +801,33 @@ describe('edge cases', () => { ...nullthrows(graph.dependencies.get(moduleFoo)).dependencies, ]).toEqual([ [ - 'qux', + expect.any(String), { absolutePath: '/qux', - data: {data: {asyncType: null, locs: []}, name: 'qux'}, + data: { + data: objectContaining({asyncType: null, locs: []}), + name: 'qux', + }, }, ], [ - 'bar', + expect.any(String), { absolutePath: '/bar', - data: {data: {asyncType: null, locs: []}, name: 'bar'}, + data: { + data: objectContaining({asyncType: null, locs: []}), + name: 'bar', + }, }, ], [ - 'baz', + expect.any(String), { absolutePath: '/baz', - data: {data: {asyncType: null, locs: []}, name: 'baz'}, + data: { + data: objectContaining({asyncType: null, locs: []}), + name: 'baz', + }, }, ], ]); @@ -1901,21 +1924,21 @@ describe('edge cases', () => { ...nullthrows(graph.dependencies.get(entryModule)).dependencies, ]).toEqual([ [ - 'foo.js', + expect.any(String), { absolutePath: '/foo', data: { - data: {asyncType: null, locs: []}, + data: objectContaining({asyncType: null, locs: []}), name: 'foo.js', }, }, ], [ - 'foo', + expect.any(String), { absolutePath: '/foo', data: { - data: {asyncType: null, locs: []}, + data: objectContaining({asyncType: null, locs: []}), name: 'foo', }, }, @@ -2007,12 +2030,12 @@ describe('edge cases', () => { [ entryModule, [ - {name: 'foo', path: moduleFoo}, - {name: 'bar', path: moduleBar}, + {name: 'foo', path: moduleFoo, data: {key: 'foo'}}, + {name: 'bar', path: moduleBar, data: {key: 'bar'}}, ], ], - [moduleFoo, [{name: 'baz', path: moduleBaz}]], - [moduleBar, [{name: 'baz', path: moduleBaz}]], + [moduleFoo, [{name: 'baz', path: moduleBaz, data: {key: 'baz'}}]], + [moduleBar, [{name: 'baz', path: moduleBaz, data: {key: 'baz'}}]], ]); // Test that even when having different modules taking longer, the order @@ -2039,6 +2062,7 @@ describe('reorderGraph', () => { data: { asyncType: null, locs: [], + key: path.substr(1), }, name: path.substr(1), }, @@ -2171,17 +2195,14 @@ describe('optional dependencies', () => { }); describe('parallel edges', () => { - it('add twice w/ same key, build and remove once', async () => { + it('add twice w/ same name, build and remove once', async () => { // Create a second edge between /foo and /bar. - Actions.addDependency('/foo', '/bar', undefined); + Actions.addDependency('/foo', '/bar', undefined, undefined, { + key: 'bar-second-key', + }); await initialTraverseDependencies(graph, options); - const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies; - const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath); - // We dedupe the dependencies because they have the same `name`. - expect(fooDepsResolved).toEqual(['/bar', '/baz']); - // Remove one of the edges between /foo and /bar (arbitrarily) Actions.removeDependency('/foo', '/bar'); @@ -2194,17 +2215,14 @@ describe('parallel edges', () => { }); }); - it('add twice w/ same key, build and remove twice', async () => { + it('add twice w/ same name, build and remove twice', async () => { // Create a second edge between /foo and /bar. - Actions.addDependency('/foo', '/bar', undefined); + Actions.addDependency('/foo', '/bar', undefined, undefined, { + key: 'bar-second-key', + }); await initialTraverseDependencies(graph, options); - const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies; - const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath); - // We dedupe the dependencies because they have the same `name`. - expect(fooDepsResolved).toEqual(['/bar', '/baz']); - // Remove both edges between /foo and /bar Actions.removeDependency('/foo', '/bar'); Actions.removeDependency('/foo', '/bar'); @@ -2218,17 +2236,12 @@ describe('parallel edges', () => { }); }); - it('add twice w/ different keys, build and remove once', async () => { + it('add twice w/ different names, build and remove once', async () => { // Create a second edge between /foo and /bar, with a different `name`. Actions.addDependency('/foo', '/bar', undefined, 'bar-second'); await initialTraverseDependencies(graph, options); - const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies; - const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath); - // We don't dedupe the dependencies because they have different `name`s. - expect(fooDepsResolved).toEqual(['/bar', '/baz', '/bar']); - // Remove one of the edges between /foo and /bar (arbitrarily) Actions.removeDependency('/foo', '/bar'); @@ -2241,17 +2254,12 @@ describe('parallel edges', () => { }); }); - it('add twice w/ different keys, build and remove twice', async () => { + it('add twice w/ different names, build and remove twice', async () => { // Create a second edge between /foo and /bar, with a different `name`. Actions.addDependency('/foo', '/bar', undefined, 'bar-second'); await initialTraverseDependencies(graph, options); - const fooDeps = nullthrows(graph.dependencies.get('/foo')).dependencies; - const fooDepsResolved = [...fooDeps.values()].map(dep => dep.absolutePath); - // We don't dedupe the dependencies because they have different `name`s. - expect(fooDepsResolved).toEqual(['/bar', '/baz', '/bar']); - // Remove both edges between /foo and /bar Actions.removeDependency('/foo', '/bar'); Actions.removeDependency('/foo', '/bar'); diff --git a/packages/metro/src/DeltaBundler/graphOperations.js b/packages/metro/src/DeltaBundler/graphOperations.js index 530d7d62b1..3d715df2da 100644 --- a/packages/metro/src/DeltaBundler/graphOperations.js +++ b/packages/metro/src/DeltaBundler/graphOperations.js @@ -295,8 +295,8 @@ async function processModule( graph.dependencies.set(module.path, module); // Diff dependencies (1/2): remove dependencies that have changed or been removed. - for (const [relativePath, prevDependency] of previousDependencies) { - const curDependency = currentDependencies.get(relativePath); + for (const [key, prevDependency] of previousDependencies) { + const curDependency = currentDependencies.get(key); if ( !curDependency || curDependency.absolutePath !== prevDependency.absolutePath || @@ -304,21 +304,14 @@ async function processModule( curDependency.data.data.asyncType !== prevDependency.data.data.asyncType) ) { - removeDependency( - module, - relativePath, - prevDependency, - graph, - delta, - options, - ); + removeDependency(module, key, prevDependency, graph, delta, options); } } // Diff dependencies (2/2): add dependencies that have changed or been added. const promises = []; - for (const [relativePath, curDependency] of currentDependencies) { - const prevDependency = previousDependencies.get(relativePath); + for (const [key, curDependency] of currentDependencies) { + const prevDependency = previousDependencies.get(key); if ( !prevDependency || prevDependency.absolutePath !== curDependency.absolutePath || @@ -327,14 +320,7 @@ async function processModule( curDependency.data.data.asyncType) ) { promises.push( - addDependency( - module, - relativePath, - curDependency, - graph, - delta, - options, - ), + addDependency(module, key, curDependency, graph, delta, options), ); } } @@ -361,7 +347,7 @@ async function processModule( async function addDependency( parentModule: Module, - relativePath: string, + key: string, dependency: Dependency, graph: Graph, delta: Delta, @@ -420,18 +406,18 @@ async function addDependency( // This means the parent's dependencies can get desynced from // inverseDependencies and the other fields in the case of lazy edges. // Not an optimal representation :( - parentModule.dependencies.set(relativePath, dependency); + parentModule.dependencies.set(key, dependency); } function removeDependency( parentModule: Module, - relativePath: string, + key: string, dependency: Dependency, graph: Graph, delta: Delta, options: InternalOptions, ): void { - parentModule.dependencies.delete(relativePath); + parentModule.dependencies.delete(key); const {absolutePath} = dependency; @@ -466,37 +452,42 @@ function resolveDependencies( dependencies: $ReadOnlyArray, options: InternalOptions, ): Map { - const resolve = (parentPath: string, result: TransformResultDependency) => { - const relativePath = result.name; + const maybeResolvedDeps = new Map(); + for (const dep of dependencies) { + let resolvedDep; try { - return [ - relativePath, - { - absolutePath: options.resolve(parentPath, relativePath), - data: result, - }, - ]; + resolvedDep = { + absolutePath: options.resolve(parentPath, dep.name), + data: dep, + }; } catch (error) { // Ignore unavailable optional dependencies. They are guarded // with a try-catch block and will be handled during runtime. - if (result.data.isOptional !== true) { + if (dep.data.isOptional !== true) { throw error; } } - return undefined; - }; + const key = dep.data.key; + if (maybeResolvedDeps.has(key)) { + throw new Error( + `resolveDependencies: Found duplicate dependency key '${key}' in ${parentPath}`, + ); + } + maybeResolvedDeps.set(key, resolvedDep); + } - const resolved = dependencies.reduce( - (list: Array<[string, Dependency]>, result: TransformResultDependency) => { - const resolvedPath = resolve(parentPath, result); - if (resolvedPath) { - list.push(resolvedPath); - } - return list; - }, - [], - ); - return new Map(resolved); + const resolvedDeps = new Map(); + // Return just the dependencies we successfully resolved. + // FIXME: This has a bad bug affecting all dependencies *after* an unresolved + // optional dependency. We'll need to propagate the nulls all the way to the + // serializer and the require() runtime to keep the dependency map from being + // desynced from the contents of the module. + for (const [key, resolvedDep] of maybeResolvedDeps) { + if (resolvedDep) { + resolvedDeps.set(key, resolvedDep); + } + } + return resolvedDeps; } /** @@ -604,8 +595,8 @@ function releaseModule( delta: Delta, options: InternalOptions, ) { - for (const [relativePath, dependency] of module.dependencies) { - removeDependency(module, relativePath, dependency, graph, delta, options); + for (const [key, dependency] of module.dependencies) { + removeDependency(module, key, dependency, graph, delta, options); } graph.privateState.gc.color.set(module.path, 'black'); if (!graph.privateState.gc.possibleCycleRoots.has(module.path)) { diff --git a/packages/metro/src/DeltaBundler/types.flow.js b/packages/metro/src/DeltaBundler/types.flow.js index f244607e12..eb0932529e 100644 --- a/packages/metro/src/DeltaBundler/types.flow.js +++ b/packages/metro/src/DeltaBundler/types.flow.js @@ -31,10 +31,13 @@ export type TransformResultDependency = { +name: string, /** - * Extra data returned by the dependency extractor. Whatever is added here is - * blindly piped by Metro to the serializers. + * Extra data returned by the dependency extractor. */ +data: { + /** + * A locally unique key for this dependency within the current module. + */ + +key: string, /** * If not null, this dependency is due to a dynamic `import()` or `__prefetchImport()` call. */ diff --git a/packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js b/packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js index 168e4ac0c2..b4187f4956 100644 --- a/packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js +++ b/packages/metro/src/ModuleGraph/worker/__tests__/collectDependencies-test.js @@ -1476,6 +1476,9 @@ class MockModuleDependencyRegistry asyncType: qualifier.asyncType, isOptional: qualifier.optional ?? false, locs: [], + + // Index = easy key for every dependency since we don't collapse/reorder + key: String(this._dependencies.length), }; if (qualifier.splitCondition) { diff --git a/packages/metro/src/ModuleGraph/worker/collectDependencies.js b/packages/metro/src/ModuleGraph/worker/collectDependencies.js index e4a7d6e3b0..cbb3f84a9a 100644 --- a/packages/metro/src/ModuleGraph/worker/collectDependencies.js +++ b/packages/metro/src/ModuleGraph/worker/collectDependencies.js @@ -52,6 +52,8 @@ export type RequireContextParams = $ReadOnly<{ }>; type DependencyData = $ReadOnly<{ + // A locally unique key for this dependency within the current module. + key: string, // If null, then the dependency is synchronous. // (ex. `require('foo')`) asyncType: AsyncDependencyType | null, @@ -777,6 +779,7 @@ class DefaultModuleDependencyRegistry asyncType: qualifier.asyncType, locs: [], index: this._dependencies.size, + key: require('crypto').createHash('sha1').update(key).digest('base64'), }; if (qualifier.optional) {