From 5f446f3e98e0ead8b62345fe13afccc53826c8b7 Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Wed, 18 Oct 2023 11:18:03 -0700 Subject: [PATCH] Fix - removing an async dependency may incorrectly free its target Summary: Metro's core `Graph` maintains a `CountingSet` of inverse "eager" dependencies for each module - zero inverse dependencies triggers a module's removal from the graph. Currently, this is not decremented with on dependency removals consistently with the way it is incremented. In particular, removing an async dependency (in combination with `options.lazy`) removes an inverse dependency, even though the reverse does not add one. This leads to incorrect deletion of module B from the graph when module A has both sync and async dependencies on B, and the async dependency is removed. ``` * **[Fix]**: Fix Fast Refresh edge case where removing an async dependency could incorrectly remove modules from the graph. ``` Reviewed By: motiz88 Differential Revision: D50309038 fbshipit-source-id: 4a44703ac31fc400736c5666a51ca26783a34d19 --- packages/metro/src/DeltaBundler/Graph.js | 9 ++- .../src/DeltaBundler/__tests__/Graph-test.js | 65 +++++++++++++++++-- .../__snapshots__/Graph-test.js.snap | 16 ++--- 3 files changed, 75 insertions(+), 15 deletions(-) diff --git a/packages/metro/src/DeltaBundler/Graph.js b/packages/metro/src/DeltaBundler/Graph.js index 50c3a23ca7..2b9a6b37a3 100644 --- a/packages/metro/src/DeltaBundler/Graph.js +++ b/packages/metro/src/DeltaBundler/Graph.js @@ -485,16 +485,19 @@ export class Graph { return; } + const module = this.dependencies.get(absolutePath); + if (options.lazy && dependency.data.data.asyncType != null) { this._decrementImportBundleReference(dependency, parentModule); + } else if (module) { + // Decrement inverseDependencies only if the dependency is not async, + // mirroring the increment conditions in _addDependency. + module.inverseDependencies.delete(parentModule.path); } - const module = this.dependencies.get(absolutePath); - if (!module) { return; } - module.inverseDependencies.delete(parentModule.path); if ( module.inverseDependencies.size > 0 || this.entryPoints.has(absolutePath) diff --git a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js index e15c826fbc..4e84c101c9 100644 --- a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js @@ -103,9 +103,10 @@ const Actions = { name?: string, data?: DependencyDataInput, } = {}, - ) { - Actions.addInferredDependency(path, dependencyPath, options); + ): string { + const key = Actions.addInferredDependency(path, dependencyPath, options); files.add(path); + return key; }, addInferredDependency( @@ -120,7 +121,7 @@ const Actions = { name?: string, data?: DependencyDataInput, } = {}, - ): void { + ): string { if (!mockedDependencies.has(path)) { Actions.createFile(path); } @@ -128,7 +129,7 @@ const Actions = { const depName = name ?? dependencyPath.replace('/', ''); const key = require('crypto') .createHash('sha1') - .update(depName) + .update([depName, data?.asyncType ?? '(null)'].join('\0')) .digest('base64'); const dep = { name: depName, @@ -149,11 +150,16 @@ const Actions = { mockedDependencyTree.set(path, deps); mockedDependencies.add(dependencyPath); + return key; }, removeDependency(path: string, dependencyPath: string) { Actions.removeInferredDependency(path, dependencyPath); + files.add(path); + }, + removeDependencyByKey(path: string, key: string) { + Actions.removeInferredDependencyByKey(path, key); files.add(path); }, @@ -166,6 +172,15 @@ const Actions = { mockedDependencyTree.set(path, deps); } }, + + removeInferredDependencyByKey(path: string, key: string) { + const deps = nullthrows(mockedDependencyTree.get(path)); + const index = deps.findIndex(({data}) => data.key === key); + if (index !== -1) { + deps.splice(index, 1); + mockedDependencyTree.set(path, deps); + } + }, }; function deferred( @@ -1545,6 +1560,48 @@ describe('edge cases', () => { }); }); + it('removing an async dependency preserves existing sync dependency', async () => { + const asyncDependencyKey = Actions.addDependency('/bundle', '/foo', { + data: { + asyncType: 'async', + }, + }); + /* + ┌─────────┐ ───▶ ┌──────┐ ┌──────┐ + │ /bundle │ │ /foo │ ──▶ │ /bar │ + └─────────┘ ···▶ └──────┘ └──────┘ + │ + │ + ▼ + ┌──────┐ + │ /baz │ + └──────┘ + */ + await graph.initialTraverseDependencies(localOptions); + files.clear(); + Actions.removeDependencyByKey('/bundle', asyncDependencyKey); + + // The synchronous dependency remains, /foo should not be removed. + /* + ┌─────────┐ ┌──────┐ ┌──────┐ + │ /bundle │ ──▶ │ /foo │ ──▶ │ /bar │ + └─────────┘ └──────┘ └──────┘ + │ + │ + ▼ + ┌──────┐ + │ /baz │ + └──────┘ + */ + expect( + getPaths(await graph.traverseDependencies([...files], localOptions)), + ).toEqual({ + added: new Set([]), + deleted: new Set([]), + modified: new Set(['/bundle']), + }); + }); + it('changing a sync dependency to async is a deletion', async () => { /* ┌─────────┐ ┌──────┐ ┌──────┐ diff --git a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap index 26b4313db5..eec94bf247 100644 --- a/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap +++ b/packages/metro/src/DeltaBundler/__tests__/__snapshots__/Graph-test.js.snap @@ -5,12 +5,12 @@ TestGraph { "dependencies": Map { "/bundle" => Object { "dependencies": Map { - "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" => Object { + "LB7P4TKrvfdUdViBXGaVopqz7Os=" => Object { "absolutePath": "/foo", "data": Object { "data": Object { "asyncType": null, - "key": "C+7Hteo/D9vJXQ3UfzxbwnXaijM=", + "key": "LB7P4TKrvfdUdViBXGaVopqz7Os=", "locs": Array [], }, "name": "foo", @@ -34,23 +34,23 @@ TestGraph { }, "/foo" => Object { "dependencies": Map { - "Ys23Ag/5IOWqZCw9QGaVDdHwH00=" => Object { + "W+de6an7x9bzpev84O0W/hS4K8U=" => Object { "absolutePath": "/bar", "data": Object { "data": Object { "asyncType": null, - "key": "Ys23Ag/5IOWqZCw9QGaVDdHwH00=", + "key": "W+de6an7x9bzpev84O0W/hS4K8U=", "locs": Array [], }, "name": "bar", }, }, - "u+lgol6jEdIdQGaek98gA7qbkKI=" => Object { + "x6e9Oz1JO0QPfIBBjUad2qqGFjI=" => Object { "absolutePath": "/baz", "data": Object { "data": Object { "asyncType": null, - "key": "u+lgol6jEdIdQGaek98gA7qbkKI=", + "key": "x6e9Oz1JO0QPfIBBjUad2qqGFjI=", "locs": Array [], }, "name": "baz", @@ -132,12 +132,12 @@ TestGraph { "dependencies": Map { "/bundle" => Object { "dependencies": Map { - "C+7Hteo/D9vJXQ3UfzxbwnXaijM=" => Object { + "LB7P4TKrvfdUdViBXGaVopqz7Os=" => Object { "absolutePath": "/foo", "data": Object { "data": Object { "asyncType": null, - "key": "C+7Hteo/D9vJXQ3UfzxbwnXaijM=", + "key": "LB7P4TKrvfdUdViBXGaVopqz7Os=", "locs": Array [], }, "name": "foo",