ModuleResolution: integrate Haste resolution as part of the overall resolution logic

Summary:
This is part of the efforts to clean up and extract the resolution logic. In this diff, the haste resolution is moved as part of the main resolution function rather than having 2 separate top-level functions. The reason for doing this is that logic is duplicated otherwise, making the already complex code harder to follow still. An example of duplicated logic is the call to `isRelativeImport`, that is done both in ResolutionRequest and ModuleResolution's node resolver. In that case, we never want to use Haste. Another duplication is the redirect of requires of package/haste names. With this changeset it is done in a single place at the beginning of the algo.

This changeset causes slight changes in behaviors. For example, consider `require('Foo/')`. A lookup of `Foo` would be done in the package `browser` field for a potential redirect. With this new code, we'll only look for `Foo/` in the redirect map. My opinion is that this is for the better, as this uniformize the way it works with Node.js `node_module` packages, making resolution more predictable. We should, additionally, actively strive to get rid of trailing slashes anywhere they might be in RN as they bring no apparent technical feature, but more confusion (I might be missing context, naturally).

Next steps will be to clean up `_resolveHasteDependency` completely, removing the callsite `try..catch` in favor of saner conditionals, and enforcing Haste packages to resolve if they exist (right now if a Haste package is found but corrupted, we just continue merrily trying to resolve the module with the rest of the logic; we should hash-crash instead, same as has been done for `resolvePackage`).

Reviewed By: cpojer

Differential Revision: D6642347

fbshipit-source-id: 2f40575b35916b644f342e0267c465a89bee202c

packages/metro/src/node-haste/DependencyGraph/ModuleResolution.js

@@ -125,25 +125,13 @@ class ModuleResolver<TModule: Moduleish, TPackage: Packageish> {
     this._options = options;
   }
 
-  resolveHasteDependency(
+  _resolveHasteDependency(
     fromModule: TModule,
     toModuleName: string,
     platform: string | null,
   ): TModule {
-    toModuleName = normalizePath(toModuleName);
-
-    const pck = fromModule.getPackage();
-    let realModuleName;
-    if (pck) {
-      /* $FlowFixMe: redirectRequire can actually return `false` for
-         exclusions*/
-      realModuleName = (pck.redirectRequire(toModuleName): string);
-    } else {
-      realModuleName = toModuleName;
-    }
-
     const modulePath = this._options.moduleMap.getModule(
-      realModuleName,
+      toModuleName,
       platform,
       /* supportsNativePlatform */ true,
     );
@@ -154,7 +142,7 @@ class ModuleResolver<TModule: Moduleish, TPackage: Packageish> {
       return module;
     }
 
-    let packageName = realModuleName;
+    let packageName = toModuleName;
     let packageJsonPath;
     while (packageName && packageName !== '.') {
       packageJsonPath = this._options.moduleMap.getPackage(
@@ -174,12 +162,12 @@ class ModuleResolver<TModule: Moduleish, TPackage: Packageish> {
       // representing the package.
       const packageDirPath = path.dirname(packageJsonPath);
       // FIXME: if we're trying to require the package main module (ie.
-      // packageName === realModuleName), don't do as if it could be a
+      // packageName === toModuleName), don't do as if it could be a
       // "FileOrDir", call directly the `resolvePackage` function! Otherwise we
       // might actually be grabbing a completely unrelated file.
       const potentialModulePath = path.join(
         packageDirPath,
-        path.relative(packageName, realModuleName),
+        path.relative(packageName, toModuleName),
       );
       return this._loadAsFileOrDirOrThrow(
         potentialModulePath,
@@ -228,9 +216,10 @@ class ModuleResolver<TModule: Moduleish, TPackage: Packageish> {
     );
   }
 
-  resolveNodeDependency(
+  resolveDependency(
     fromModule: TModule,
     toModuleName: string,
+    allowHaste: boolean,
     platform: string | null,
   ): TModule {
     if (isRelativeImport(toModuleName) || isAbsolutePath(toModuleName)) {
@@ -254,6 +243,22 @@ class ModuleResolver<TModule: Moduleish, TPackage: Packageish> {
       return this._resolveFileOrDir(fromModule, absPath, platform);
     }
 
+    // At that point we only have module names that
+    // aren't relative paths nor absolute paths.
+    if (allowHaste) {
+      try {
+        return this._resolveHasteDependency(
+          fromModule,
+          normalizePath(realModuleName),
+          platform,
+        );
+      } catch (error) {
+        if (!(error instanceof UnableToResolveError)) {
+          throw error;
+        }
+      }
+    }
+
     const searchQueue = [];
     for (
       let currDir = path.dirname(fromModule.path);

packages/metro/src/node-haste/DependencyGraph/ResolutionRequest.js

@@ -89,33 +89,17 @@ class ResolutionRequest<TModule: Moduleish, TPackage: Packageish> {
     const resolver = this._options.moduleResolver;
     const platform = this._options.platform;
 
-    if (
-      isAbsolutePath(toModuleName) ||
-      isRelativeImport(toModuleName) ||
-      this._options.helpers.isNodeModulesDir(fromModule.path)
-    ) {
-      return cacheResult(
-        resolver.resolveNodeDependency(fromModule, toModuleName, platform),
-      );
-    }
+    const allowHaste = !this._options.helpers.isNodeModulesDir(fromModule.path);
 
-    return cacheResult(
-      tryResolveSync(
-        () => this._resolveHasteDependency(fromModule, toModuleName, platform),
-        () =>
-          resolver.resolveNodeDependency(fromModule, toModuleName, platform),
-      ),
-    );
-  }
-
-  _resolveHasteDependency(
-    fromModule: TModule,
-    toModuleName: string,
-    platform: string | null,
-  ): TModule {
-    const rs = this._options.moduleResolver;
     try {
-      return rs.resolveHasteDependency(fromModule, toModuleName, platform);
+      return cacheResult(
+        resolver.resolveDependency(
+          fromModule,
+          toModuleName,
+          allowHaste,
+          platform,
+        ),
+      );
     } catch (error) {
       if (error instanceof DuplicateHasteCandidatesError) {
         throw new AmbiguousModuleResolutionError(fromModule.path, error);
@@ -137,22 +121,6 @@ function getResolutionCacheKey(modulePath, depName) {
   return `${path.resolve(modulePath)}:${depName}`;
 }
 
-/**
- * It may not be a great pattern to leverage exception just for "trying" things
- * out, notably for performance. We should consider replacing these functions
- * to be nullable-returning, or being better stucture to the algorithm.
- */
-function tryResolveSync<T>(action: () => T, secondaryAction: () => T): T {
-  try {
-    return action();
-  } catch (error) {
-    if (!(error instanceof ModuleResolution.UnableToResolveError)) {
-      throw error;
-    }
-    return secondaryAction();
-  }
-}
-
 class AmbiguousModuleResolutionError extends Error {
   fromModulePath: string;
   hasteError: DuplicateHasteCandidatesError;