Only consider directory candidates when resolving path specifiers end…

…ing in a `/`

Summary:
When an import uses a relative (or absolute) module path specifier ending in a `/`, e.g. `require('./foo/')`, we should *not* consider `./foo.js` to be a candidate. The `/` implies `foo` must be a (symlink to a) directory.

In the edge case where *both* `./foo.js` and `./foo/index.js` exist, Metro will currently incorrectly resolve `./foo/` to the former.

Verified that this differs from Node.js and Webpack's `resolve`.

```
 - **[Fix]:** Resolver: Only consider directory candidates when resolving path specifiers ending in a `/`
```

NOTE: This *could* be considered a breaking change, if so I'll hold it for RN 0.76. I'd argue though that it corrects a misalignment in Metro that could currently cause third-party (especially non-RN) packages to resolve incorrectly vs Node and other bundlers, and so it's a fix we'd potentially consider backporting.

Reviewed By: GijsWeterings

Differential Revision: D60731286

fbshipit-source-id: 60ea791e7cd65b597ae4d13955adc60192f107b8

packages/metro-resolver/src/__tests__/index-test.js

@@ -20,6 +20,7 @@ const Resolver = require('../index');
 
 const fileMap = {
   '/root/project/foo.js': '',
+  '/root/project/foo/index.js': '',
   '/root/project/bar.js': '',
   '/root/smth/beep.js': '',
   '/root/node_modules/apple/package.json': JSON.stringify({
@@ -89,13 +90,33 @@ it('resolves a relative path', () => {
   });
 });
 
+it('resolves a relative path ending in a slash as a directory', () => {
+  expect(Resolver.resolve(CONTEXT, './foo/', null)).toEqual({
+    type: 'sourceFile',
+    filePath: '/root/project/foo/index.js',
+  });
+});
+
 it('resolves a relative path in another folder', () => {
   expect(Resolver.resolve(CONTEXT, '../smth/beep', null)).toEqual({
     type: 'sourceFile',
     filePath: '/root/smth/beep.js',
   });
 });
 
+it('does not resolve a relative path ending in a slash as a file', () => {
+  expect(() => Resolver.resolve(CONTEXT, './bar/', null)).toThrow(
+    new FailedToResolvePathError({
+      file: null,
+      dir: {
+        type: 'sourceFile',
+        filePathPrefix: '/root/project/bar/index',
+        candidateExts: ['', '.js', '.jsx', '.json', '.ts', '.tsx'],
+      },
+    }),
+  );
+});
+
 it('resolves a package in `node_modules`', () => {
   expect(Resolver.resolve(CONTEXT, 'apple', null)).toEqual({
     type: 'sourceFile',

packages/metro-resolver/src/errors/FailedToResolvePathError.js

@@ -21,8 +21,10 @@ class FailedToResolvePathError extends Error {
   constructor(candidates: FileAndDirCandidates) {
     super(
       'The module could not be resolved because none of these files exist:\n\n' +
-        `  * ${formatFileCandidates(candidates.file)}\n` +
-        `  * ${formatFileCandidates(candidates.dir)}`,
+        [candidates.file, candidates.dir]
+          .filter(Boolean)
+          .map(candidates => `  * ${formatFileCandidates(candidates)}`)
+          .join('\n'),
     );
     this.candidates = candidates;
   }

packages/metro-resolver/src/resolve.js

@@ -171,15 +171,25 @@ function resolveModulePath(
 
   const dirPath = path.dirname(redirectedPath);
   const fileName = path.basename(redirectedPath);
-  const fileResult = resolveFile(context, dirPath, fileName, platform);
-  if (fileResult.type === 'resolved') {
+
+  const fileResult: ?Result<Resolution, FileCandidates> =
+    // require('./foo/') should never resolve to ./foo.js - a trailing slash
+    // implies we should resolve as a directory only.
+    redirectedPath.endsWith(path.sep)
+      ? null
+      : resolveFile(context, dirPath, fileName, platform);
+
+  if (fileResult != null && fileResult.type === 'resolved') {
     return fileResult;
   }
   const dirResult = resolvePackageEntryPoint(context, redirectedPath, platform);
   if (dirResult.type === 'resolved') {
     return dirResult;
   }
-  return failedFor({file: fileResult.candidates, dir: dirResult.candidates});
+  return failedFor({
+    file: fileResult?.candidates ?? null,
+    dir: dirResult.candidates,
+  });
 }
 
 /**
@@ -234,8 +244,10 @@ class MissingFileInHastePackageError extends Error {
         `the Haste package \`${opts.packageName}\` was found. However the ` +
         `module \`${opts.pathInModule}\` could not be found within ` +
         'the package. Indeed, none of these files exist:\n\n' +
-        `  * \`${formatFileCandidates(opts.candidates.file)}\`\n` +
-        `  * \`${formatFileCandidates(opts.candidates.dir)}\``,
+        [opts.candidates.file, opts.candidates.dir]
+          .filter(Boolean)
+          .map(candidates => `  * \`${formatFileCandidates(candidates)}\``)
+          .join('\n'),
     );
     Object.assign(this, opts);
   }

packages/metro-resolver/src/types.js

@@ -31,8 +31,8 @@ export type AssetResolution = $ReadOnly<{
 export type FileResolution = AssetResolution | SourceFileResolution;
 
 export type FileAndDirCandidates = {
-  +dir: FileCandidates,
-  +file: FileCandidates,
+  +dir: ?FileCandidates,
+  +file: ?FileCandidates,
 };
 
 /**

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

@@ -197,15 +197,14 @@ class ModuleResolver<TPackage: Packageish> {
         throw new UnableToResolveError(
           fromModule.path,
           dependency.name,
-          [
-            '\n\nNone of these files exist:',
-            `  * ${Resolver.formatFileCandidates(
-              this._removeRoot(candidates.file),
-            )}`,
-            `  * ${Resolver.formatFileCandidates(
-              this._removeRoot(candidates.dir),
-            )}`,
-          ].join('\n'),
+          '\n\nNone of these files exist:\n' +
+            [candidates.file, candidates.dir]
+              .filter(Boolean)
+              .map(
+                candidates =>
+                  `  * ${Resolver.formatFileCandidates(this._removeRoot(candidates))}`,
+              )
+              .join('\n'),
           {
             cause: error,
             dependency,