experimentalImportBundleSupport: Retraverse parents of deleted async dependencies

motiz88 · facebook-github-bot · commit cb806d15d4c4 · 2022-10-27T08:31:33.000-07:00
Summary:
Changelog: [Experimental]

Correctly invalidates resolved async dependencies when their target files are deleted, by tracking inverse dependencies of entries in `importBundleNames` ( = async dependencies not necessarily tracked in the `dependencies` set).

## Problem

Suppose we have the following two files

```
// /src/a.js

import('b');
```

```
// /src/b.js

export default 42;
```

described by the following dependency graph:

```
┌───────────┐  import('b')   ┌───────────┐
│ /src/a.js │ ─────────────▶ │ /src/b.js │
└───────────┘                └───────────┘
```

Now suppose we delete `/src/b.js`:
* If we delete it and immediately perform a build, we'd expect the resolver to throw an error.
* If we recreate `b.js` in a different directory ( = move it) we'd expect the resolution to be updated. (For the sake of this example, assume we're using Haste resolution.)

In short, this needs to work the way sync dependencies work: the inverse dependencies of a deleted node must be invalidated so that their dependencies are resolved anew in the next build. But under `experimentalImportBundleSupport: true`, `b.js` is not tracked as a node in the `dependencies` set, so the code in `DeltaCalculator` that is responsible for this does not apply.

## Solution

Here, we create a `getModifiedModulesForDeletedPath` method on `Graph`, which:

1. Encapsulates what `DeltaCalculator` previously did by directly reading `dependencies`.
2. Also returns the inverse dependencies of async modules that may not exist in the `dependencies` set.

(2) is achieved by tracking resolved async modules as nodes in a separate set (`importBundleNodes`). These nodes cannot have their own dependencies, so they can't introduce cycles and therefore don't need any new handling in terms of GC. We repurpose the existing logic that maintained `importBundleNames` and expose an `importBundleNames` getter for compatibility.

NOTE: Ultimately, I'm planning to remove the `importBundleNames` kludge entirely and instead serialise the resolved paths of async dependencies inside each parent module's dependency array. The current diff directly enables that by preventing improper caching of the paths. (But as explained above, this improper caching is *already* a bug regardless.)

Reviewed By: jacdebug

Differential Revision: D40463613

fbshipit-source-id: 35c73b0ae8c50d742bd08386d0247fa1e337d6b0
diff --git a/packages/metro/src/DeltaBundler/DeltaCalculator.js b/packages/metro/src/DeltaBundler/DeltaCalculator.js
@@ -246,16 +246,14 @@ class DeltaCalculator<T> extends EventEmitter {
     // If a file has been deleted, we want to invalidate any other file that
     // depends on it, so we can process it and correctly return an error.
     deletedFiles.forEach((filePath: string) => {
-      const module = this._graph.dependencies.get(filePath);
-
-      if (module) {
-        module.inverseDependencies.forEach((path: string) => {
-          // Only mark the inverse dependency as modified if it's not already
-          // marked as deleted (in that case we can just ignore it).
-          if (!deletedFiles.has(path)) {
-            modifiedFiles.add(path);
-          }
-        });
+      for (const path of this._graph.getModifiedModulesForDeletedPath(
+        filePath,
+      )) {
+        // Only mark the inverse dependency as modified if it's not already
+        // marked as deleted (in that case we can just ignore it).
+        if (!deletedFiles.has(path)) {
+          modifiedFiles.add(path);
+        }
       }
     });
 
diff --git a/packages/metro/src/DeltaBundler/Graph.js b/packages/metro/src/DeltaBundler/Graph.js
@@ -26,7 +26,7 @@
  * 2. We keep the "root buffer" (possibleCycleRoots) free of duplicates by
  *    making it a Set, instead of storing a "buffered" flag on each node.
  * 3. On top of tracking edges between nodes, we also count references between
- *    nodes and entries in the importBundleNames set.
+ *    nodes and entries in the importBundleNodes set.
  */
 
 import type {RequireContextParams} from '../ModuleGraph/worker/collectDependencies';
@@ -123,19 +123,25 @@ export class Graph<T = MixedOutput> {
   +entryPoints: $ReadOnlySet<string>;
   +transformOptions: TransformInputOptions;
   +dependencies: Dependencies<T> = new Map();
-  +importBundleNames: Set<string> = new Set();
+  +#importBundleNodes: Map<
+    string,
+    $ReadOnly<{
+      inverseDependencies: CountingSet<string>,
+    }>,
+  > = new Map();
+
+  // $FlowIgnore[unsafe-getters-setters]
+  get importBundleNames(): $ReadOnlySet<string> {
+    return new Set(this.#importBundleNodes.keys());
+  }
 
   /// GC state for nodes in the graph (this.dependencies)
-  #gc: {
+  +#gc: {
     +color: Map<string, NodeColor>,
     +possibleCycleRoots: Set<string>,
-
-    // Reference counts for entries in importBundleNames
-    +importBundleRefs: Map<string, number>,
   } = {
     color: new Map(),
     possibleCycleRoots: new Set(),
-    importBundleRefs: new Map(),
   };
 
   /** Resolved context parameters from `require.context`. */
@@ -219,14 +225,10 @@ export class Graph<T = MixedOutput> {
       this.dependencies.size === 0,
       'initialTraverseDependencies called on nonempty graph',
     );
-    invariant(
-      this.importBundleNames.size === 0,
-      'initialTraverseDependencies called on nonempty graph',
-    );
 
     this.#gc.color.clear();
     this.#gc.possibleCycleRoots.clear();
-    this.#gc.importBundleRefs.clear();
+    this.#importBundleNodes.clear();
 
     for (const path of this.entryPoints) {
       // Each entry point implicitly has a refcount of 1, so mark them all black.
@@ -361,8 +363,8 @@ export class Graph<T = MixedOutput> {
     ) {
       // Don't add a node for the module if we are traversing async dependencies
       // lazily (and this is an async dependency). Instead, record it in
-      // importBundleNames.
-      this._incrementImportBundleReference(dependency);
+      // importBundleNodes.
+      this._incrementImportBundleReference(dependency, parentModule);
     } else {
       if (!module) {
         // Add a new node to the graph.
@@ -419,7 +421,7 @@ export class Graph<T = MixedOutput> {
       options.experimentalImportBundleSupport &&
       dependency.data.data.asyncType != null
     ) {
-      this._decrementImportBundleReference(dependency);
+      this._decrementImportBundleReference(dependency, parentModule);
     }
 
     const module = this.dependencies.get(absolutePath);
@@ -455,6 +457,16 @@ export class Graph<T = MixedOutput> {
     }
   }
 
+  /**
+   * Gets the list of modules affected by the deletion of a given file. The
+   * caller is expected to mark these modules as modified in the next call to
+   * traverseDependencies. Note that the list may contain duplicates.
+   */
+  *getModifiedModulesForDeletedPath(filePath: string): Iterable<string> {
+    yield* this.dependencies.get(filePath)?.inverseDependencies ?? [];
+    yield* this.#importBundleNodes.get(filePath)?.inverseDependencies ?? [];
+  }
+
   _resolveDependencies(
     parentPath: string,
     dependencies: $ReadOnlyArray<TransformResultDependency>,
@@ -588,32 +600,36 @@ export class Graph<T = MixedOutput> {
 
   /** Garbage collection functions */
 
-  // Add an entry to importBundleNames (or increase the reference count of an existing one)
-  _incrementImportBundleReference(dependency: Dependency) {
+  // Add an entry to importBundleNodes (or record an inverse dependency of an existing one)
+  _incrementImportBundleReference(
+    dependency: Dependency,
+    parentModule: Module<T>,
+  ) {
     const {absolutePath} = dependency;
-
-    this.#gc.importBundleRefs.set(
-      absolutePath,
-      (this.#gc.importBundleRefs.get(absolutePath) ?? 0) + 1,
-    );
-    this.importBundleNames.add(absolutePath);
+    const importBundleNode = this.#importBundleNodes.get(absolutePath) ?? {
+      inverseDependencies: new CountingSet(),
+    };
+    importBundleNode.inverseDependencies.add(parentModule.path);
+    this.#importBundleNodes.set(absolutePath, importBundleNode);
   }
 
-  // Decrease the reference count of an entry in importBundleNames (and delete it if necessary)
-  _decrementImportBundleReference(dependency: Dependency) {
+  // Decrease the reference count of an entry in importBundleNodes (and delete it if necessary)
+  _decrementImportBundleReference(
+    dependency: Dependency,
+    parentModule: Module<T>,
+  ) {
     const {absolutePath} = dependency;
 
-    const prevRefCount = nullthrows(
-      this.#gc.importBundleRefs.get(absolutePath),
+    const importBundleNode = nullthrows(
+      this.#importBundleNodes.get(absolutePath),
     );
     invariant(
-      prevRefCount > 0,
-      'experimentalImportBundleSupport: import bundle refcount not valid',
+      importBundleNode.inverseDependencies.has(parentModule.path),
+      'experimentalImportBundleSupport: import bundle inverse references',
     );
-    this.#gc.importBundleRefs.set(absolutePath, prevRefCount - 1);
-    if (prevRefCount === 1) {
-      this.#gc.importBundleRefs.delete(absolutePath);
-      this.importBundleNames.delete(absolutePath);
+    importBundleNode.inverseDependencies.delete(parentModule.path);
+    if (importBundleNode.inverseDependencies.size === 0) {
+      this.#importBundleNodes.delete(absolutePath);
     }
   }
 
diff --git a/packages/metro/src/DeltaBundler/__tests__/Graph-test.js b/packages/metro/src/DeltaBundler/__tests__/Graph-test.js
@@ -69,13 +69,16 @@ const Actions = {
     }
   },
 
-  moveFile(from: string, to: string) {
+  moveFile(from: string, to: string, graph: Graph<>) {
     Actions.createFile(to);
-    Actions.deleteFile(from);
+    Actions.deleteFile(from, graph);
   },
 
-  deleteFile(path: string) {
+  deleteFile(path: string, graph: Graph<>) {
     mockedDependencies.delete(path);
+    for (const modifiedPath of graph.getModifiedModulesForDeletedPath(path)) {
+      Actions.modifyFile(modifiedPath);
+    }
   },
 
   createFile(path: string) {
@@ -327,7 +330,7 @@ beforeEach(async () => {
       const {path} = deps.filter(dep => dep.name === to)[0];
 
       if (!mockedDependencies.has(path)) {
-        throw new Error(`Dependency not found: ${path}->${to}`);
+        throw new Error(`Dependency not found: ${from} -> ${path}`);
       }
       return path;
     },
@@ -442,7 +445,7 @@ it('should return added/removed dependencies', async () => {
 it('should retry to traverse the dependencies as it was after getting an error', async () => {
   await graph.initialTraverseDependencies(options);
 
-  Actions.deleteFile(moduleBar);
+  Actions.deleteFile(moduleBar, graph);
 
   await expect(
     graph.traverseDependencies(['/foo'], options),
@@ -587,7 +590,7 @@ describe('edge cases', () => {
     await graph.initialTraverseDependencies(options);
 
     Actions.removeDependency('/foo', '/baz');
-    Actions.moveFile('/baz', '/qux');
+    Actions.moveFile('/baz', '/qux', graph);
     Actions.addDependency('/foo', '/qux');
 
     expect(
@@ -606,7 +609,7 @@ describe('edge cases', () => {
     Actions.addDependency('/bundle', '/foo-renamed');
     Actions.removeDependency('/bundle', '/foo');
 
-    Actions.moveFile('/foo', '/foo-renamed');
+    Actions.moveFile('/foo', '/foo-renamed', graph);
     Actions.addDependency('/foo-renamed', '/bar');
     Actions.addDependency('/foo-renamed', '/baz');
 
@@ -1975,6 +1978,70 @@ describe('edge cases', () => {
       });
       expect(graph.importBundleNames).toEqual(new Set());
     });
+
+    it('deleting the target of an async dependency retraverses its parent', async () => {
+      Actions.removeDependency('/bundle', '/foo');
+      Actions.addDependency('/bundle', '/foo', {
+        data: {
+          asyncType: 'async',
+        },
+      });
+
+      /*
+      ┌─────────┐  async   ┌──────┐     ┌──────┐
+      │ /bundle │ ·······▶ │ /foo │ ──▶ │ /bar │
+      └─────────┘          └──────┘     └──────┘
+                            │
+                            │
+                            ▼
+                          ┌──────┐
+                          │ /baz │
+                          └──────┘
+      */
+      await graph.initialTraverseDependencies(localOptions);
+      files.clear();
+
+      Actions.deleteFile('/foo', graph);
+
+      /*
+      ┌─────────┐  async   ┌┄┄╲┄╱┄┐     ┌──────┐
+      │ /bundle │ ·······▶ ┆ /foo ┆ ──▶ │ /bar │
+      └─────────┘          └┄┄╱┄╲┄┘     └──────┘
+                            │
+                            │
+                            ▼
+                          ┌──────┐
+                          │ /baz │
+                          └──────┘
+      */
+      await expect(
+        graph.traverseDependencies([...files], localOptions),
+      ).rejects.toThrowError('Dependency not found: /bundle -> /foo');
+
+      // NOTE: not clearing `files`, to mimic DeltaCalculator's error behaviour.
+
+      Actions.createFile('/foo');
+
+      /*
+      ┌─────────┐  async   ┏━━━━━━┓     ┌──────┐
+      │ /bundle │ ·······▶ ┃ /foo ┃ ──▶ │ /bar │
+      └─────────┘          ┗━━━━━━┛     └──────┘
+                            │
+                            │
+                            ▼
+                          ┌──────┐
+                          │ /baz │
+                          └──────┘
+      */
+      expect(
+        getPaths(await graph.traverseDependencies([...files], localOptions)),
+      ).toEqual({
+        added: new Set([]),
+        modified: new Set(['/bundle']),
+        deleted: new Set([]),
+      });
+      expect(graph.importBundleNames).toEqual(new Set(['/foo']));
+    });
   });
 
   it('should try to transform every file only once', async () => {
@@ -2362,7 +2429,7 @@ describe('require.context', () => {
     ]).toEqual([ctxPath]);
 
     // Delete the matched file
-    Actions.deleteFile('/ctx/matched-file');
+    Actions.deleteFile('/ctx/matched-file', graph);
 
     // Propagate the deletion to the context module (normally DeltaCalculator's responsibility)
     Actions.removeInferredDependency(ctxPath, '/ctx/matched-file');
@@ -2876,12 +2943,12 @@ describe('optional dependencies', () => {
     Actions.addDependency('/bundle-o', '/regular-a');
     Actions.addDependency('/bundle-o', '/optional-b');
 
-    Actions.deleteFile('/optional-b');
-
     localGraph = new TestGraph({
       entryPoints: new Set(['/bundle-o']),
       transformOptions: options.transformOptions,
     });
+
+    Actions.deleteFile('/optional-b', localGraph);
   });
 
   it('missing optional dependency will be skipped', 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
@@ -112,7 +112,6 @@ TestGraph {
   "entryPoints": Set {
     "/bundle",
   },
-  "importBundleNames": Set {},
   "transformOptions": Object {
     "dev": false,
     "hot": false,
@@ -160,7 +159,6 @@ TestGraph {
   "entryPoints": Set {
     "/bundle",
   },
-  "importBundleNames": Set {},
   "transformOptions": Object {
     "dev": false,
     "hot": false,
