Fix optional dependencies shifting dependency indices, allow nulls in dependency arrays

packages/metro-runtime/src/polyfills/__tests__/require-test.js

@@ -505,6 +505,38 @@ describe('require', () => {
       );
     });
 
+    test('throws "module not found" when trying to require an unresolvable optional dependency', () => {
+      createModuleSystem(moduleSystem, false, '');
+
+      createModule(
+        moduleSystem,
+        0,
+        'foo.js',
+        (global, require, importDefault, importAll, module) => {
+          require(null);
+        },
+      );
+
+      expect(() => moduleSystem.__r(0)).toThrow('Cannot find module');
+    });
+
+    test('throws "module not found" with name when trying to require an unresolvable optional dependency', () => {
+      createModuleSystem(moduleSystem, true, '');
+
+      createModule(
+        moduleSystem,
+        0,
+        'foo.js',
+        (global, require, importDefault, importAll, module) => {
+          require(null, './not-exists');
+        },
+      );
+
+      expect(() => moduleSystem.__r(0)).toThrow(
+        "Cannot find module './not-exists'",
+      );
+    });
+
     test('throws an error when a module throws an error', () => {
       createModuleSystem(moduleSystem, false, '');
 

packages/metro-runtime/src/polyfills/require.js

@@ -169,7 +169,19 @@ function define(
   }
 }
 
-function metroRequire(moduleId: ModuleID | VerboseModuleNameForDev): Exports {
+function metroRequire(
+  moduleId: ModuleID | VerboseModuleNameForDev | null,
+  maybeNameForDev?: string,
+): Exports {
+  // Unresolved optional dependencies are nulls in dependency maps
+  // eslint-disable-next-line lint/strictly-null
+  if (moduleId === null) {
+    if (__DEV__ && typeof maybeNameForDev === 'string') {
+      throw new Error("Cannot find module '" + maybeNameForDev + "'");
+    }
+    throw new Error('Cannot find module');
+  }
+
   if (__DEV__ && typeof moduleId === 'string') {
     const verboseName = moduleId;
     moduleId = getModuleIdForVerboseName(verboseName);

packages/metro/src/DeltaBundler/Graph.js

@@ -39,11 +39,13 @@ import type {
   Module,
   ModuleData,
   Options,
+  ResolvedDependency,
   TransformInputOptions,
 } from './types.flow';
 
 import {fileMatchesContext} from '../lib/contextModule';
 import CountingSet from '../lib/CountingSet';
+import {isResolvedDependency} from '../lib/isResolvedDependency';
 import {buildSubgraph} from './buildSubgraph';
 
 const invariant = require('invariant');
@@ -121,7 +123,7 @@ function getInternalOptions<T>({
 }
 
 function isWeakOrLazy<T>(
-  dependency: Dependency,
+  dependency: ResolvedDependency,
   options: InternalOptions<T>,
 ): boolean {
   const asyncType = dependency.data.data.asyncType;
@@ -355,7 +357,7 @@ export class Graph<T = MixedOutput> {
         options.onDependencyAdded();
         return result;
       },
-      shouldTraverse: (dependency: Dependency) => {
+      shouldTraverse: (dependency: ResolvedDependency) => {
         if (options.shallow || isWeakOrLazy(dependency, options)) {
           return false;
         }
@@ -510,14 +512,11 @@ export class Graph<T = MixedOutput> {
     delta: Delta<T>,
     options: InternalOptions<T>,
   ): void {
-    const path = dependency.absolutePath;
-
-    // The module may already exist, in which case we just need to update some
-    // bookkeeping instead of adding a new node to the graph.
-    let module = this.dependencies.get(path);
-
     if (options.shallow) {
       // Don't add a node for the module if the graph is shallow (single-module).
+    } else if (!isResolvedDependency(dependency)) {
+      // If the dependency is a missing optional dependency, it has no node of
+      // its own. We just need to add it to the parent's dependency map.
     } else if (dependency.data.data.asyncType === 'weak') {
       // Exclude weak dependencies from the bundle.
     } else if (options.lazy && dependency.data.data.asyncType != null) {
@@ -526,6 +525,11 @@ export class Graph<T = MixedOutput> {
       // importBundleNodes.
       this._incrementImportBundleReference(dependency, parentModule);
     } else {
+      // The module may already exist, in which case we just need to update some
+      // bookkeeping instead of adding a new node to the graph.
+      const path = dependency.absolutePath;
+      let module = this.dependencies.get(path);
+
       if (!module) {
         try {
           module = this._recursivelyCommitModule(path, delta, options);
@@ -550,12 +554,15 @@ export class Graph<T = MixedOutput> {
       this._markModuleInUse(module);
     }
 
-    if (requireContext) {
-      this.#resolvedContexts.set(path, requireContext);
-    } else {
-      // This dependency may have existed previously as a require.context -
-      // clean it up.
-      this.#resolvedContexts.delete(path);
+    if (isResolvedDependency(dependency)) {
+      const path = dependency.absolutePath;
+      if (requireContext) {
+        this.#resolvedContexts.set(path, requireContext);
+      } else {
+        // This dependency may have existed previously as a require.context -
+        // clean it up.
+        this.#resolvedContexts.delete(path);
+      }
     }
 
     // Update the parent's dependency map unless we failed to add a dependency.
@@ -574,13 +581,16 @@ export class Graph<T = MixedOutput> {
   ): void {
     parentModule.dependencies.delete(key);
 
-    const {absolutePath} = dependency;
-
-    if (dependency.data.data.asyncType === 'weak') {
-      // Weak dependencies are excluded from the bundle.
+    if (
+      !isResolvedDependency(dependency) ||
+      dependency.data.data.asyncType === 'weak'
+    ) {
+      // Weak and unresolved dependencies are excluded from the bundle.
       return;
     }
 
+    const {absolutePath} = dependency;
+
     const module = this.dependencies.get(absolutePath);
 
     if (options.lazy && dependency.data.data.asyncType != null) {
@@ -671,8 +681,12 @@ export class Graph<T = MixedOutput> {
       orderedDependencies.set(module.path, module);
     }
 
-    module.dependencies.forEach((dependency: Dependency) => {
+    module.dependencies.forEach(dependency => {
       const path = dependency.absolutePath;
+      if (path == null) {
+        // If the dependency is not a missing optional dependency, it has no children to reorder.
+        return;
+      }
       const childModule = this.dependencies.get(path);
 
       if (!childModule) {
@@ -691,7 +705,7 @@ export class Graph<T = MixedOutput> {
 
   // Add an entry to importBundleNodes (or record an inverse dependency of an existing one)
   _incrementImportBundleReference(
-    dependency: Dependency,
+    dependency: ResolvedDependency,
     parentModule: Module<T>,
   ) {
     const {absolutePath} = dependency;
@@ -704,7 +718,7 @@ export class Graph<T = MixedOutput> {
 
   // Decrease the reference count of an entry in importBundleNodes (and delete it if necessary)
   _decrementImportBundleReference(
-    dependency: Dependency,
+    dependency: ResolvedDependency,
     parentModule: Module<T>,
   ) {
     const {absolutePath} = dependency;
@@ -734,7 +748,10 @@ export class Graph<T = MixedOutput> {
     options: InternalOptions<T>,
   ): Iterator<Module<T>> {
     for (const dependency of module.dependencies.values()) {
-      if (isWeakOrLazy(dependency, options)) {
+      if (
+        !isResolvedDependency(dependency) ||
+        isWeakOrLazy(dependency, options)
+      ) {
         continue;
       }
       yield nullthrows(this.dependencies.get(dependency.absolutePath));
@@ -747,6 +764,9 @@ export class Graph<T = MixedOutput> {
 
     const resolvedContexts: Map<string, RequireContext> = new Map();
     for (const [key, dependency] of dependencies) {
+      if (!isResolvedDependency(dependency)) {
+        continue;
+      }
       const resolvedContext = this.#resolvedContexts.get(
         dependency.absolutePath,
       );
@@ -783,6 +803,10 @@ export class Graph<T = MixedOutput> {
     }
 
     for (const [key, dependency] of module.dependencies) {
+      if (!isResolvedDependency(dependency)) {
+        // If the dependency is not a missing optional dependency, it has no children to remove.
+        continue;
+      }
       this._removeDependency(module, key, dependency, delta, options);
     }
     this.#gc.color.set(module.path, 'black');
@@ -894,6 +918,9 @@ export class Graph<T = MixedOutput> {
     if (color === 'white' && !this.#gc.possibleCycleRoots.has(module.path)) {
       this.#gc.color.set(module.path, 'black');
       for (const dependency of module.dependencies.values()) {
+        if (!isResolvedDependency(dependency)) {
+          continue;
+        }
         const childModule = this.dependencies.get(dependency.absolutePath);
         // The child may already have been collected.
         if (childModule) {

packages/metro/src/DeltaBundler/Serializers/helpers/getTransitiveDependencies.js

@@ -13,6 +13,8 @@
 
 import type {ReadOnlyGraph} from '../../types.flow';
 
+import {isResolvedDependency} from '../../../lib/isResolvedDependency';
+
 function getTransitiveDependencies<T>(
   path: string,
   graph: ReadOnlyGraph<T>,
@@ -44,7 +46,9 @@ function _getDeps<T>(
   deps.add(path);
 
   for (const dependency of module.dependencies.values()) {
-    _getDeps(dependency.absolutePath, graph, deps);
+    if (isResolvedDependency(dependency)) {
+      _getDeps(dependency.absolutePath, graph, deps);
+    }
   }
 
   return deps;

packages/metro/src/DeltaBundler/Serializers/helpers/js.js

@@ -14,6 +14,7 @@
 import type {MixedOutput, Module} from '../../types.flow';
 import type {JsOutput} from 'metro-transform-worker';
 
+const {isResolvedDependency} = require('../../../lib/isResolvedDependency');
 const invariant = require('invariant');
 const jscSafeUrl = require('jsc-safe-url');
 const {addParamsToDefineCall} = require('metro-transform-plugins');
@@ -47,6 +48,11 @@ function getModuleParams(module: Module<>, options: Options): Array<mixed> {
   let hasPaths = false;
   const dependencyMapArray = Array.from(module.dependencies.values()).map(
     dependency => {
+      if (!isResolvedDependency(dependency)) {
+        // An unresolved dependency, which should cause a runtime error
+        // when required.
+        return null;
+      }
       const id = options.createModuleId(dependency.absolutePath);
       if (options.includeAsyncPaths && dependency.data.data.asyncType != null) {
         hasPaths = true;