From 29c77bff31e2475a086bc3f04073f485da8f9ff0 Mon Sep 17 00:00:00 2001 From: Alex Hunt Date: Wed, 5 Apr 2023 11:27:47 -0700 Subject: [PATCH] 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 --- .../src/PackageExportsResolve.js | 11 ---- .../src/__tests__/package-exports-test.js | 54 +++++++++---------- .../metro-resolver/src/__tests__/utils.js | 3 ++ .../src/errors/InvalidModuleSpecifierError.js | 36 ------------- packages/metro-resolver/src/resolve.js | 6 +-- .../metro/src/node-haste/DependencyGraph.js | 5 ++ 6 files changed, 35 insertions(+), 80 deletions(-) delete mode 100644 packages/metro-resolver/src/errors/InvalidModuleSpecifierError.js diff --git a/packages/metro-resolver/src/PackageExportsResolve.js b/packages/metro-resolver/src/PackageExportsResolve.js index a79e783d59..e6a0a282fb 100644 --- a/packages/metro-resolver/src/PackageExportsResolve.js +++ b/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, diff --git a/packages/metro-resolver/src/__tests__/package-exports-test.js b/packages/metro-resolver/src/__tests__/package-exports-test.js index 0cb4234184..5c5e84d70e 100644 --- a/packages/metro-resolver/src/__tests__/package-exports-test.js +++ b/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(); diff --git a/packages/metro-resolver/src/__tests__/utils.js b/packages/metro-resolver/src/__tests__/utils.js index 13ac1ba0bc..ec786114a2 100644 --- a/packages/metro-resolver/src/__tests__/utils.js +++ b/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); diff --git a/packages/metro-resolver/src/errors/InvalidModuleSpecifierError.js b/packages/metro-resolver/src/errors/InvalidModuleSpecifierError.js deleted file mode 100644 index cdc7253303..0000000000 --- a/packages/metro-resolver/src/errors/InvalidModuleSpecifierError.js +++ /dev/null @@ -1,36 +0,0 @@ -/** - * Copyright (c) Meta Platforms, Inc. and affiliates. - * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. - * - * @flow strict - * @format - * @oncall react_native - */ - -export default class InvalidModuleSpecifierError extends Error { - /** - * Either the import specifier read, or the absolute path of the module being - * resolved (used when import specifier is externally remapped). - */ - importSpecifier: string; - - /** - * The description of the error cause. - */ - reason: string; - - constructor( - opts: $ReadOnly<{ - importSpecifier: string, - reason: string, - }>, - ) { - super( - `Invalid import specifier ${opts.importSpecifier}.\nReason: ` + - opts.reason, - ); - Object.assign(this, opts); - } -} diff --git a/packages/metro-resolver/src/resolve.js b/packages/metro-resolver/src/resolve.js index 92acd3cc7f..1f847a8b06 100644 --- a/packages/metro-resolver/src/resolve.js +++ b/packages/metro-resolver/src/resolve.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.', ); diff --git a/packages/metro/src/node-haste/DependencyGraph.js b/packages/metro/src/node-haste/DependencyGraph.js index 2dbff2466a..98c5d28aa5 100644 --- a/packages/metro/src/node-haste/DependencyGraph.js +++ b/packages/metro/src/node-haste/DependencyGraph.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;