Fix getPackageForModule implementation to avoid edge cases in resolver

Summary:
Fixes an awkward bug where, while attempting package resolution against candidate `node_modules` paths, paths which don't exist are short-circuited to the parent package if present. Because Package Exports resolution has the side-effect of logging a warning for an invalid package path (`PackagePathNotExportedError`), repeat `resolvePackage` calls under this scenario (to apparent subpaths including `/node_modules/`) would log incorrect warnings to the terminal.

More specifically, this is because `context.getPackageForModule` uses a different resolution strategy to the top-level `resolve` function (originating from the `redirectModulePath` design). This produces a mismatch where we may eagerly locate a parent package. Independently, we should address this disparity in future.

Does not affect [`"browser"` spec](https://github.com/defunctzombie/package-browser-field-spec) / `mainFields` resolution, since the `redirectModulePath` approach bypasses the above `node_modules` lookup strategy in the simple case.

Changelog: **[Experimental]** Fix bug where Package Exports warnings may have been logged for nested `node_modules` path candidates

Reviewed By: motiz88

Differential Revision: D44149246

fbshipit-source-id: 43df6885e712a93f9d07e8fb8e2e36132a766fc8

Author: huntie

packages/metro-resolver/src/PackageExportsResolve.js

@@ -18,7 +18,6 @@ import type {
 } from './types';
 
 import path from 'path';
-import InvalidModuleSpecifierError from './errors/InvalidModuleSpecifierError';
 import InvalidPackageConfigurationError from './errors/InvalidPackageConfigurationError';
 import PackagePathNotExportedError from './errors/PackagePathNotExportedError';
 import resolveAsset from './resolveAsset';
@@ -91,16 +90,6 @@ export function resolvePackageTargetFromExports(
       );
     }
 
-    if (patternMatch != null && findInvalidPathSegment(patternMatch) != null) {
-      throw new InvalidModuleSpecifierError({
-        importSpecifier: modulePath,
-        reason:
-          `The target for "${subpath}" defined in "exports" is "${target}", ` +
-          'however this expands to an invalid subpath because the pattern ' +
-          `match "${patternMatch}" is invalid.`,
-      });
-    }
-
     const filePath = path.join(
       packagePath,
       patternMatch != null ? target.replace('*', patternMatch) : target,

packages/metro-resolver/src/__tests__/package-exports-test.js

@@ -254,6 +254,7 @@ describe('with package exports resolution enabled', () => {
         '/root/node_modules/test-pkg/lib/foo.ios.js': '',
         '/root/node_modules/test-pkg/private/bar.js': '',
         '/root/node_modules/test-pkg/node_modules/baz/index.js': '',
+        '/root/node_modules/test-pkg/node_modules/baz/package.json': '',
         '/root/node_modules/test-pkg/metadata.json': '',
         '/root/node_modules/test-pkg/metadata.min.json': '',
       }),
@@ -347,6 +348,31 @@ describe('with package exports resolution enabled', () => {
       });
     });
 
+    test('should resolve subpath when package is located in nested node_modules path', () => {
+      const logWarning = jest.fn();
+      const context = {
+        ...baseContext,
+        ...createPackageAccessors({
+          '/root/node_modules/test-pkg/package.json': {
+            exports: './index-exports.js',
+          },
+          '/root/node_modules/test-pkg/node_modules/baz/package.json': {
+            exports: './index.js',
+          },
+        }),
+        originModulePath: '/root/node_modules/test-pkg/private/bar.js',
+        unstable_logWarning: logWarning,
+      };
+
+      expect(Resolver.resolve(context, 'baz', null)).toEqual({
+        type: 'sourceFile',
+        filePath: '/root/node_modules/test-pkg/node_modules/baz/index.js',
+      });
+      // If a warning was logged, we have incorrectly tried to resolve "exports"
+      // against the parent package.json.
+      expect(logWarning).not.toHaveBeenCalled();
+    });
+
     test('should expand array of strings as subpath mapping (root shorthand)', () => {
       const logWarning = jest.fn();
       const context = {
@@ -509,34 +535,6 @@ describe('with package exports resolution enabled', () => {
       `);
     });
 
-    test('[nonstrict] should fall back and log warning for an invalid pattern match substitution', () => {
-      const logWarning = jest.fn();
-      const context = {
-        ...baseContext,
-        unstable_logWarning: logWarning,
-      };
-
-      // TODO(T145206395): Improve this error trace
-      expect(() =>
-        Resolver.resolve(
-          context,
-          'test-pkg/features/node_modules/foo/index.js',
-          null,
-        ),
-      ).toThrowErrorMatchingInlineSnapshot(`
-        "Module does not exist in the Haste module map or in these directories:
-          /root/src/node_modules
-          /root/node_modules
-          /node_modules
-        "
-      `);
-      expect(logWarning).toHaveBeenCalledTimes(1);
-      expect(logWarning.mock.calls[0][0]).toMatchInlineSnapshot(`
-        "Invalid import specifier /root/node_modules/test-pkg/features/node_modules/foo/index.js.
-        Reason: The target for \\"./features/node_modules/foo/index.js\\" defined in \\"exports\\" is \\"./src/features/*.js\\", however this expands to an invalid subpath because the pattern match \\"node_modules/foo/index\\" is invalid. Falling back to file-based resolution."
-      `);
-    });
-
     describe('package encapsulation', () => {
       test('[nonstrict] should fall back to "browser" spec resolution and log inaccessible import warning', () => {
         const logWarning = jest.fn();

packages/metro-resolver/src/__tests__/utils.js

@@ -102,6 +102,9 @@ export function createPackageAccessors(
     let dir = path.join(parsedPath.dir, parsedPath.name);
 
     do {
+      if (path.basename(dir) === 'node_modules') {
+        return null;
+      }
       const candidate = path.join(dir, 'package.json');
       const packageJson = getPackage(candidate);
 

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

@@ -22,7 +22,6 @@ import type {
 import path from 'path';
 import FailedToResolveNameError from './errors/FailedToResolveNameError';
 import FailedToResolvePathError from './errors/FailedToResolvePathError';
-import InvalidModuleSpecifierError from './errors/InvalidModuleSpecifierError';
 import InvalidPackageConfigurationError from './errors/InvalidPackageConfigurationError';
 import InvalidPackageError from './errors/InvalidPackageError';
 import PackagePathNotExportedError from './errors/PackagePathNotExportedError';
@@ -280,10 +279,7 @@ function resolvePackage(
               ' Falling back to file-based resolution. Consider updating the ' +
               'call site or asking the package maintainer(s) to expose this API.',
           );
-        } else if (
-          e instanceof InvalidModuleSpecifierError ||
-          e instanceof InvalidPackageConfigurationError
-        ) {
+        } else if (e instanceof InvalidPackageConfigurationError) {
           context.unstable_logWarning(
             e.message + ' Falling back to file-based resolution.',
           );

packages/metro-resolver/src/resolve.js

@@ -154,6 +154,11 @@ class DependencyGraph extends EventEmitter {
     const root = parsedPath.root;
     let dir = parsedPath.dir;
     do {
+      // If we've hit a node_modules directory, the closest package was not
+      // found (`filePath` was likely nonexistent).
+      if (path.basename(dir) === 'node_modules') {
+        return null;
+      }
       const candidate = path.join(dir, 'package.json');
       if (this._fileSystem.exists(candidate)) {
         return candidate;