From 4e9cc4b6dda9e9158c3cf276d05aace4aefe0d8e Mon Sep 17 00:00:00 2001 From: Rob Hogan Date: Mon, 14 Oct 2024 06:12:48 -0700 Subject: [PATCH] Disallow Haste modules with separators, and require Haste packages to have standard valid package names Summary: Currently, with `allowHaste`, an input `moduleName` of `foo/bar/baz` will call `resolveHasteModule` with `foo/bar/baz` and `resolveHastePackage` with each of `foo/bar/baz`, `foo/bar` and `foo` until a match is found. This is quite inefficient at the top of the resolver algorithm, especially for deeply nested paths. We can tidy up a bit here by narrowing the definition of a Haste module name and a Haste package name. - A Haste module name must not contain path separators - A Haste package name must be a valid (npm) package name, i.e. `'simple'` or `'scoped/package'`. ``` - **[Breaking]**: Disallow Haste modules with separators and Haste packages with invalid package names. ``` Reviewed By: huntie Differential Revision: D64323177 fbshipit-source-id: 0f3e0342beb13499fc91f086b26aff339f96e877 --- .../src/__tests__/index-test.js | 58 +++++++++++- packages/metro-resolver/src/resolve.js | 88 +++++++++++-------- 2 files changed, 108 insertions(+), 38 deletions(-) diff --git a/packages/metro-resolver/src/__tests__/index-test.js b/packages/metro-resolver/src/__tests__/index-test.js index ea5c98e4a..e3ab2f04f 100644 --- a/packages/metro-resolver/src/__tests__/index-test.js +++ b/packages/metro-resolver/src/__tests__/index-test.js @@ -313,6 +313,21 @@ test('resolves Haste modules', () => { }); }); +test('does not call resolveHasteModule for a specifier with separators', () => { + const resolveHasteModule = jest.fn(); + expect(() => + Resolver.resolve( + { + ...CONTEXT, + resolveHasteModule, + }, + 'Foo/bar', + null, + ), + ).toThrow(); + expect(resolveHasteModule).not.toHaveBeenCalled(); +}); + test('resolves a Haste package', () => { expect(Resolver.resolve(CONTEXT, 'some-package', null)).toEqual({ type: 'sourceFile', @@ -320,6 +335,47 @@ test('resolves a Haste package', () => { }); }); +test.each([ + ['simple', 'simple'], + ['simple/with/subpath', 'simple'], + ['@scoped/package', '@scoped/package'], + ['@scoped/with/subpath', '@scoped/with'], +])( + 'calls resolveHastePackage for specifier %s with %s', + (specifier, expectedHastePackageCandidate) => { + const resolveHastePackage = jest.fn(); + expect(() => + Resolver.resolve( + { + ...CONTEXT, + resolveHastePackage, + }, + specifier, + null, + ), + ).toThrow(); + expect(resolveHastePackage).toHaveBeenCalledWith( + expectedHastePackageCandidate, + ); + expect(resolveHastePackage).toHaveBeenCalledTimes(1); + }, +); + +test('does not call resolveHastePackage for invalid specifier @notvalid', () => { + const resolveHastePackage = jest.fn(); + expect(() => + Resolver.resolve( + { + ...CONTEXT, + resolveHastePackage, + }, + '@notvalid', + null, + ), + ).toThrow(); + expect(resolveHastePackage).not.toHaveBeenCalled(); +}); + test('resolves a file inside a Haste package', () => { expect( Resolver.resolve(CONTEXT, 'some-package/subdir/other-file', null), @@ -333,7 +389,7 @@ test('throws a descriptive error when a file inside a Haste package cannot be re expect(() => { Resolver.resolve(CONTEXT, 'some-package/subdir/does-not-exist', null); }).toThrowErrorMatchingInlineSnapshot(` - "While resolving module \`some-package/subdir/does-not-exist\`, the Haste package \`some-package\` was found. However the module \`subdir/does-not-exist\` could not be found within the package. Indeed, none of these files exist: + "While resolving module \`some-package/subdir/does-not-exist\`, the Haste package \`some-package\` was found. However the subpath \`./subdir/does-not-exist\` could not be found within the package. Indeed, none of these files exist: * \`/haste/some-package/subdir/does-not-exist(.js|.jsx|.json|.ts|.tsx)\` * \`/haste/some-package/subdir/does-not-exist\`" diff --git a/packages/metro-resolver/src/resolve.js b/packages/metro-resolver/src/resolve.js index ff9691b75..ab75aeffb 100644 --- a/packages/metro-resolver/src/resolve.js +++ b/packages/metro-resolver/src/resolve.js @@ -32,6 +32,15 @@ import resolveAsset from './resolveAsset'; import isAssetFile from './utils/isAssetFile'; import path from 'path'; +type ParsedBareSpecifier = $ReadOnly<{ + isSinglePart: boolean, + isValidPackageName: boolean, + firstPart: string, + normalizedSpecifier: string, + packageName: string, + posixSubpath: string, +}>; + function resolve( context: ResolutionContext, moduleName: string, @@ -94,14 +103,24 @@ function resolve( /** * At this point, realModuleName is not a "direct" (absolute or relative) - * import, so it's either Haste name or a package specifier. + * import, so it's a bare specifier - for our purposes either Haste name + * or a package specifier. */ + const parsedSpecifier = parseBareSpecifier(realModuleName); + if (context.allowHaste) { - const normalizedName = normalizePath(realModuleName); - const result = resolveHasteName(context, normalizedName, platform); - if (result.type === 'resolved') { - return result.resolution; + if (parsedSpecifier.isSinglePart) { + const result = context.resolveHasteModule(parsedSpecifier.firstPart); + if (result != null) { + return {type: 'sourceFile', filePath: result}; + } + } + if (parsedSpecifier.isValidPackageName) { + const result = resolveHastePackage(context, parsedSpecifier, platform); + if (result.type === 'resolved') { + return result.resolution; + } } } @@ -131,8 +150,6 @@ function resolve( const extraPaths = []; - const parsedSpecifier = parsePackageSpecifier(realModuleName); - const {extraNodeModules} = context; if (extraNodeModules && extraNodeModules[parsedSpecifier.packageName]) { const newPackageName = extraNodeModules[parsedSpecifier.packageName]; @@ -179,35 +196,51 @@ function resolve( throw new FailedToResolveNameError(nodeModulesPaths, extraPaths); } -function parsePackageSpecifier(specifier: string) { +function parseBareSpecifier(specifier: string): ParsedBareSpecifier { const normalized = path.sep === '/' ? specifier : specifier.replaceAll('\\', '/'); const firstSepIdx = normalized.indexOf('/'); if (normalized.startsWith('@') && firstSepIdx !== -1) { const secondSepIdx = normalized.indexOf('/', firstSepIdx + 1); if (secondSepIdx === -1) { + // @foo/bar (valid scoped, no subpath) return { + isSinglePart: false, + isValidPackageName: true, firstPart: normalized.slice(0, firstSepIdx), + normalizedSpecifier: normalized, packageName: normalized, posixSubpath: '.', }; } + // @foo/bar[/subpath] (valid scoped with subpath) return { + isSinglePart: false, + isValidPackageName: true, firstPart: normalized.slice(0, firstSepIdx), + normalizedSpecifier: normalized, packageName: normalized.slice(0, secondSepIdx), posixSubpath: '.' + normalized.slice(secondSepIdx), }; } + // foo or @foo, no subpath. Valid if doesn't start with '@'. if (firstSepIdx === -1) { return { + isSinglePart: true, + isValidPackageName: !normalized.startsWith('@'), firstPart: normalized, + normalizedSpecifier: normalized, packageName: normalized, posixSubpath: '.', }; } const packageName = normalized.slice(0, firstSepIdx); + // foo/subpath, valid, not scoped, with subpath return { + isSinglePart: false, + isValidPackageName: true, firstPart: packageName, + normalizedSpecifier: normalized, packageName, posixSubpath: '.' + normalized.slice(firstSepIdx), }; @@ -260,31 +293,22 @@ function resolveModulePath( } /** - * Resolve a module as a Haste module or package. For example we might try to - * resolve `Foo`, that is provided by file `/smth/Foo.js`. Or, in the case of - * a Haste package, it could be `/smth/Foo/index.js`. + * Resolve a specifier as a Haste package. */ -function resolveHasteName( +function resolveHastePackage( context: ResolutionContext, - moduleName: string, + { + normalizedSpecifier: moduleName, + packageName, + posixSubpath: pathInModule, + }: ParsedBareSpecifier, platform: string | null, ): Result { - const modulePath = context.resolveHasteModule(moduleName); - if (modulePath != null) { - return resolvedAs({type: 'sourceFile', filePath: modulePath}); - } - let packageName = moduleName; - let packageJsonPath = context.resolveHastePackage(packageName); - while (packageJsonPath == null && packageName && packageName !== '.') { - packageName = path.dirname(packageName); - packageJsonPath = context.resolveHastePackage(packageName); - } + const packageJsonPath = context.resolveHastePackage(packageName); if (packageJsonPath == null) { return failedFor(); } - const packageDirPath = path.dirname(packageJsonPath); - const pathInModule = moduleName.substring(packageName.length + 1); - const potentialModulePath = path.join(packageDirPath, pathInModule); + const potentialModulePath = path.join(packageJsonPath, '..', pathInModule); const result = resolvePackage(context, potentialModulePath, platform); if (result.type === 'resolved') { return result; @@ -309,7 +333,7 @@ class MissingFileInHastePackageError extends Error { super( `While resolving module \`${opts.moduleName}\`, ` + `the Haste package \`${opts.packageName}\` was found. However the ` + - `module \`${opts.pathInModule}\` could not be found within ` + + `subpath \`${opts.pathInModule}\` could not be found within ` + 'the package. Indeed, none of these files exist:\n\n' + [opts.candidates.file, opts.candidates.dir] .filter(Boolean) @@ -588,16 +612,6 @@ function isRelativeImport(filePath: string) { return /^[.][.]?(?:[/]|$)/.test(filePath); } -function normalizePath(modulePath: any | string) { - if (path.sep === '/') { - modulePath = path.normalize(modulePath); - } else if (path.posix) { - modulePath = path.posix.normalize(modulePath); - } - - return modulePath.replace(/\/$/, ''); -} - function resolvedAs( resolution: TResolution, ): Result {