diff --git a/packages/metro/src/DeltaBundler/Graph.js b/packages/metro/src/DeltaBundler/Graph.js index 8c6f644e81..5bb0a5ced6 100644 --- a/packages/metro/src/DeltaBundler/Graph.js +++ b/packages/metro/src/DeltaBundler/Graph.js @@ -81,6 +81,9 @@ export type Result = { * modified ones (which is useful for things like Hot Module Reloading). **/ type Delta = $ReadOnly<{ + // `added` and `deleted` are mutually exclusive. + // Internally, a module can be in both `modified` and (either) `added` or + // `deleted`. We fix this up before returning the delta to the client. added: Set, modified: Set, deleted: Set, @@ -198,8 +201,8 @@ export class Graph { const modified = new Map>(); for (const path of delta.modified) { - // Only report a module as modified if we're not already reporting it as added. - if (!delta.added.has(path)) { + // Only report a module as modified if we're not already reporting it as added or deleted. + if (!delta.added.has(path) && !delta.deleted.has(path)) { modified.set(path, nullthrows(this.dependencies.get(path))); } } @@ -381,7 +384,6 @@ export class Graph { } else { // Mark the addition in the added set. delta.added.add(path); - delta.modified.delete(path); } delta.earlyInverseDependencies.set(path, new CountingSet()); @@ -638,18 +640,15 @@ export class Graph { this.#gc.color.set(module.path, 'black'); } - // Delete an unreachable module from the graph immediately, unless it's queued - // for later deletion as a potential cycle root. Delete the module's outbound - // edges. + // Delete an unreachable module (and its outbound edges) from the graph + // immediately. // Called when the reference count of a module has reached 0. _releaseModule(module: Module, delta: Delta, options: InternalOptions) { for (const [key, dependency] of module.dependencies) { this._removeDependency(module, key, dependency, delta, options); } this.#gc.color.set(module.path, 'black'); - if (!this.#gc.possibleCycleRoots.has(module.path)) { - this._freeModule(module, delta); - } + this._freeModule(module, delta); } // Delete an unreachable module from the graph. @@ -660,7 +659,6 @@ export class Graph { } else { // Mark the deletion in the deleted set. delta.deleted.add(module.path); - delta.modified.delete(module.path); } // This module is not used anywhere else! We can clear it from the bundle. diff --git a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js index 3623081eeb..4fa61fc507 100644 --- a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js +++ b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js @@ -2254,6 +2254,71 @@ describe('edge cases', () => { deleted: new Set(['/foo', '/bar', '/baz']), }); }); + + it('deleting a cycle root, then reintroducing the same module, does not corrupt its dependencies', async () => { + Actions.createFile('/quux'); + Actions.removeDependency('/foo', '/baz'); + Actions.addDependency('/bar', '/foo'); + Actions.addDependency('/bundle', '/baz'); + Actions.addDependency('/foo', '/quux'); + files.clear(); + + /* + ┌─────────┐ ┌──────┐ ┌───────┐ ┌──────┐ + │ /bundle │ ──▶ │ /baz │ │ │ ──▶ │ /bar │ + └─────────┘ └──────┘ │ /foo │ └──────┘ + │ │ │ │ + └────────────────────────▶ │ │ ◀─────┘ + └───────┘ + │ + │ + ▼ + ┌───────┐ + │ /quux │ + └───────┘ + */ + await graph.initialTraverseDependencies(options); + + // This is a regression test for a bug: Originally `/quux` would get deleted + // incorrectly as a result of `/foo` temporarily being unreachable (and not + // itself marked for traversal, which would have "rediscovered" `/quux`). + + // The following exact order of operations reproduced the bug: + Actions.removeDependency('/bar', '/foo'); // (1) + // ^ Deletes an inbound edge while there's at least another one remaining, + // which marks `/foo` as a possible cycle root. + + Actions.removeDependency('/bundle', '/foo'); // (2) + // ^ Leaves `/foo` with no inbound edges. With the bug, this would delete + // `/foo`'s dependencies immediately but defer freeing `/foo` itself until + // the cycle collection pass. + + Actions.addDependency('/baz', '/foo'); // (3) + // ^ `/foo` has an inbound edge again! If we'd freed `/quux` in (2), it + // would now be missing. + + /* + ┌─────────┐ ┌──────┐ (3) ┌───────┐ ┌──────┐ + │ /bundle │ ──▶ │ /baz │ ━━▶ │ │ ──▶ │ /bar │ + └─────────┘ └──────┘ │ /foo │ └──────┘ + ┆ / │ │ \ ┆ + └┈┈┈┈┈┈┈┈┈/┈┈┈┈┈┈┈┈┈┈┈┈┈┈▷ │ │ ◁┈┈\┈┈┘ (1) + / (2) └───────┘ \ + │ + │ + ▼ + ┌───────┐ + │ /quux │ + └───────┘ + */ + expect( + getPaths(await graph.traverseDependencies([...files], options)), + ).toEqual({ + added: new Set([]), + modified: new Set(['/bundle', '/bar', '/baz']), + deleted: new Set([]), + }); + }); }); describe('require.context', () => {