Skip to content

Commit

Permalink
Disallow Haste modules with separators, and require Haste packages to…
Browse files Browse the repository at this point in the history 
… 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
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Oct 14, 2024
1 parent c1c80c7 commit 4e9cc4b
Show file tree
Hide file tree
Showing 2 changed files with 108 additions and 38 deletions.
58 changes: 57 additions & 1 deletion packages/metro-resolver/src/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -313,13 +313,69 @@ 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',
filePath: '/haste/some-package/main.js',
});
});

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),
Expand All @@ -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\`"
Expand Down
88 changes: 51 additions & 37 deletions packages/metro-resolver/src/resolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
}
}
}

Expand Down Expand Up @@ -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];
Expand Down Expand Up @@ -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),
};
Expand Down Expand Up @@ -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<Resolution, void> {
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;
Expand All @@ -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)
Expand Down Expand Up @@ -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<TResolution, TCandidates>(
resolution: TResolution,
): Result<TResolution, TCandidates> {
Expand Down

0 comments on commit 4e9cc4b

Please sign in to comment.