Skip to content

Commit

Permalink
Don't use context.redirectModulePath in default resolver, use brows…
Browse files Browse the repository at this point in the history 
…er spec implementation directly

Summary:
`redirectModulePath`'s API is highly inefficient, often necessitating multiple lookups of package scope - there's no way of knowing from the return value what kind of redirection has occurred, or of using the closest package looked-up, or of providing an already-looked-up package. It does not fit well with implementing `package.json#exports` resolution.

This changes the way we use `context.redirectModulePath`, so that the default resolution algorithm absorbs our implementation of `BROWSER_SPEC_REDIRECTION` and doesn't call the user-overridable `context.redirectModulePath`.

By using a fixed implementation rather than having to use a potentially-overriden black box, we can apply these redirections in a more structured way. We are *not* seeking to remove support for the `defunctzombie` spec at this point.

## Ecosystem impact
From a GH code search, `rnx-kit/metro-resolver-symlinks` is the only OSS package that [calls `context.redirectModulePath`](https://github.com/microsoft/rnx-kit/blob/%40rnx-kit/metro-resolver-symlinks%400.2.1/packages/metro-resolver-symlinks/src/resolver.ts#L58), expecting the default implementation. This change does not break anything there. There are no other callsites in popular repos.

Importantly, no public ecosystem uses *override* `redirectModulePath` by providing their own implementation. For users wanting that level of control, overriding or chaining `resolveRequest` still provides it.

Changelog:
```
 - **[Breaking]**: Don't call `context.redirectModulePath` from default resolver, use a fixed browser field spec implementation.
```

Reviewed By: huntie

Differential Revision: D64304411

fbshipit-source-id: 631bfa1fa82f8de68defbd7feea3ac0fe86c66f0
  • Loading branch information
@robhogan @facebook-github-bot
robhogan authored and facebook-github-bot committed Oct 14, 2024
1 parent 108383b commit 5e96d17
Show file tree
Hide file tree
Showing 4 changed files with 54 additions and 39 deletions.
14 changes: 6 additions & 8 deletions docs/Resolution.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ Parameters: (*context*, *moduleName*, *platform*)
2. Otherwise, attempt to resolve *moduleName* as a path
1. Let *absoluteModuleName* be the result of prepending the current directory (i.e. parent of [`context.originModulePath`](#originmodulepath-string)) with *moduleName*.
2. Return the result of [**RESOLVE_MODULE**](#resolve_module)(*context*, *absoluteModuleName*, *platform*), or continue.
3. Apply [redirections](#redirectmodulepath-string--string--false) to *moduleName*. If this results in an [empty module](#empty-module), then
3. Apply [**BROWSER_SPEC_REDIRECTION**](#browser_spec_redirection) to *moduleName*. If this is `false`:
1. Return the empty module.
4. If [Haste resolutions are allowed](#allowhaste-boolean), then
1. Get the result of [**RESOLVE_HASTE**](#resolve_haste)(*context*, *moduleName*, *platform*).
Expand All @@ -90,7 +90,7 @@ Parameters: (*context*, *moduleName*, *platform*)

Parameters: (*context*, *moduleName*, *platform*)

1. Let *filePath* be the result of applying [redirections](#redirectmodulepath-string--string--false) to *moduleName*. This may locate a replacement subpath from a containing `package.json` file based on the [`browser` field spec](https://github.com/defunctzombie/package-browser-field-spec).
1. Let *filePath* be the result of applying [**BROWSER_SPEC_REDIRECTION**](#browser_spec_redirection) to *moduleName*. This may locate a replacement subpath from a containing `package.json` file based on the [`browser` field spec](https://github.com/defunctzombie/package-browser-field-spec).
2. Return the result of [**RESOLVE_FILE**](#resolve_file)(*context*, *filePath*, *platform*), or continue.
3. Otherwise, let *dirPath* be the directory path of *filePath*.
4. If a file *dirPath* + `'package.json'` exists, resolve based on the [`browser` field spec](https://github.com/defunctzombie/package-browser-field-spec):
Expand Down Expand Up @@ -130,7 +130,7 @@ Parameters: (*context*, *filePath*, *platform*)
1. If the path refers to an [asset](#assetexts-readonlysetstring), then
1. Return the result of [**RESOLVE_ASSET**](#resolve_asset)(*context*, *filePath*, *platform*).
2. Otherwise, if the path [exists](#doesfileexist-string--boolean), then
1. Try all platform and extension variants in sequence. Return a [source file resolution](#source-file) for the first one that [exists](#doesfileexist-string--boolean) after applying [redirections](#redirectmodulepath-string--string--false). For example, if _platform_ is `android` and [`context.sourceExts`](#sourceexts-readonlyarraystring) is `['js', 'jsx']`, try this sequence of potential file names:
1. Try all platform and extension variants in sequence. Return a [source file resolution](#source-file) for the first one that [exists](#doesfileexist-string--boolean) after applying [**BROWSER_SPEC_REDIRECTION**](#browser_spec_redirection). For example, if _platform_ is `android` and [`context.sourceExts`](#sourceexts-readonlyarraystring) is `['js', 'jsx']`, try this sequence of potential file names:
1. _moduleName_ + `'.android.js'`
2. _moduleName_ + `'.native.js'` (if [`context.preferNativePlatform`](#prefernativeplatform-boolean) is `true`)
3. _moduleName_ + `'.js'`
Expand Down Expand Up @@ -213,14 +213,12 @@ By default this is set to [`resolver.nodeModulesPaths`](./Configuration.md#nodem

If `true`, try `.native.${ext}` before `.${ext}` and after `.${platform}.${ext}` during resolution. Metro sets this to `true`.

#### `redirectModulePath: string => string | false`

Rewrites a module path, or returns `false` to redirect to the special [empty module](#empty-module). In the default resolver, the resolution algorithm terminates with an [empty module result](#empty-module) if `redirectModulePath` returns `false`.

Metro uses this to implement the `package.json` [`browser` field spec](https://github.com/defunctzombie/package-browser-field-spec), particularly the ability to [replace](https://github.com/defunctzombie/package-browser-field-spec#replace-specific-files---advanced) and [ignore](https://github.com/defunctzombie/package-browser-field-spec#ignore-a-module) specific files.
#### `redirectModulePath: string => string | false` <div class="label deprecated">Deprecated</div>

The default implementation of this function is specified by [**BROWSER_SPEC_REDIRECTION**](#browser_spec_redirection).

Metro's default resolver does not call this function, instead using the [**BROWSER_SPEC_REDIRECTION**](#browser_spec_redirection) implementation directly. It is exposed here for backwards-compatible use by custom resolvers, but is considered deprecated and will be removed in a future release.

#### `resolveAsset: (dirPath: string, assetName: string, extension: string) => ?$ReadOnlyArray<string>`

Given a directory path, the base asset name and an extension, returns a list of all the asset file names that match the given base name in that directory, or `null` if no such files are found. The default implementation considers each of [`resolver.assetResolutions`](./Configuration.md#assetresolutions) and uses the `${assetName}@${resolution}${extension}` format for asset variant file names.
Expand Down
2 changes: 0 additions & 2 deletions packages/metro-resolver/src/PackageResolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ export function getPackageEntryPoint(
*
* Implements legacy (non-exports) package resolution behaviour based on the
* ["browser" field spec](https://github.com/defunctzombie/package-browser-field-spec).
*
* This is the default implementation of `context.redirectModulePath`.
*/
export function redirectModulePath(
context: $ReadOnly<{
Expand Down
67 changes: 43 additions & 24 deletions packages/metro-resolver/src/__tests__/index-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ import type {ResolutionContext} from '../index';

import {createResolutionContext} from './utils';

const FailedToResolvePathError = require('../errors/FailedToResolvePathError');
const Resolver = require('../index');
let Resolver = require('../index');

const fileMap = {
'/root/project/foo.js': '',
Expand Down Expand Up @@ -107,7 +106,7 @@ test('resolves a relative path in another folder', () => {

test('does not resolve a relative path ending in a slash as a file', () => {
expect(() => Resolver.resolve(CONTEXT, './bar/', null)).toThrow(
new FailedToResolvePathError({
new Resolver.FailedToResolvePathError({
file: null,
dir: {
type: 'sourceFile',
Expand Down Expand Up @@ -137,7 +136,7 @@ test('fails to resolve a relative path', () => {
Resolver.resolve(CONTEXT, './apple', null);
throw new Error('should not reach');
} catch (error) {
if (!(error instanceof FailedToResolvePathError)) {
if (!(error instanceof Resolver.FailedToResolvePathError)) {
throw error;
}
expect(error.candidates).toEqual({
Expand Down Expand Up @@ -209,7 +208,10 @@ test('does not resolve to additional `node_modules` if `nodeModulesPaths` is not
});

test('uses `nodeModulesPaths` to find additional node_modules not in the direct path', () => {
const context = {...CONTEXT, nodeModulesPaths: ['/other-root/node_modules']};
const context = {
...CONTEXT,
nodeModulesPaths: ['/other-root/node_modules'],
};
expect(Resolver.resolve(context, 'banana', null)).toEqual({
type: 'sourceFile',
filePath: '/other-root/node_modules/banana/main.js',
Expand Down Expand Up @@ -339,12 +341,29 @@ test('throws a descriptive error when a file inside a Haste package cannot be re
});

describe('redirectModulePath', () => {
const redirectModulePath = jest.fn();
const context = {...CONTEXT, redirectModulePath};
const mockRedirectModulePath = jest.fn();
const context = CONTEXT;

beforeEach(() => {
redirectModulePath.mockReset();
redirectModulePath.mockImplementation(filePath => false);
mockRedirectModulePath.mockReset();
mockRedirectModulePath.mockImplementation(filePath => false);

jest.resetModules();
jest.mock('../PackageResolve', () => {
return {
...jest.requireActual('../PackageResolve'),
redirectModulePath: (_ctx, specifier) =>
mockRedirectModulePath(specifier),
};
});

Resolver = require('../index');
});

afterEach(() => {
jest.unmock('../PackageResolve');
jest.resetModules();
Resolver = require('../index');
});

test('is used for relative path requests', () => {
Expand All @@ -353,8 +372,8 @@ describe('redirectModulePath', () => {
"type": "empty",
}
`);
expect(redirectModulePath).toBeCalledTimes(1);
expect(redirectModulePath).toBeCalledWith('/root/project/bar');
expect(mockRedirectModulePath).toBeCalledTimes(1);
expect(mockRedirectModulePath).toBeCalledWith('/root/project/bar');
});

test('is used for absolute path requests', () => {
Expand All @@ -363,8 +382,8 @@ describe('redirectModulePath', () => {
"type": "empty",
}
`);
expect(redirectModulePath).toBeCalledTimes(1);
expect(redirectModulePath).toBeCalledWith('/bar');
expect(mockRedirectModulePath).toBeCalledTimes(1);
expect(mockRedirectModulePath).toBeCalledWith('/bar');
});

test('is used for non-Haste package requests', () => {
Expand All @@ -374,12 +393,12 @@ describe('redirectModulePath', () => {
"type": "empty",
}
`);
expect(redirectModulePath).toBeCalledTimes(1);
expect(redirectModulePath).toBeCalledWith('does-not-exist');
expect(mockRedirectModulePath).toBeCalledTimes(1);
expect(mockRedirectModulePath).toBeCalledWith('does-not-exist');
});

test('can be used to redirect to an arbitrary relative module', () => {
redirectModulePath
test('can redirect to an arbitrary relative module', () => {
mockRedirectModulePath
.mockImplementationOnce(filePath => '../smth/beep')
.mockImplementation(filePath => filePath);
expect(Resolver.resolve(context, 'does-not-exist', null))
Expand All @@ -389,7 +408,7 @@ describe('redirectModulePath', () => {
"type": "sourceFile",
}
`);
expect(redirectModulePath.mock.calls).toMatchInlineSnapshot(`
expect(mockRedirectModulePath.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"does-not-exist",
Expand All @@ -405,7 +424,7 @@ describe('redirectModulePath', () => {
});

test("is called for source extension candidates that don't exist on disk", () => {
redirectModulePath.mockImplementation(filePath =>
mockRedirectModulePath.mockImplementation(filePath =>
filePath.replace('.another-fake-ext', '.js'),
);
expect(
Expand All @@ -420,7 +439,7 @@ describe('redirectModulePath', () => {
"type": "sourceFile",
}
`);
expect(redirectModulePath.mock.calls).toMatchInlineSnapshot(`
expect(mockRedirectModulePath.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"/root/smth/beep",
Expand All @@ -436,7 +455,7 @@ describe('redirectModulePath', () => {
});

test('can resolve to empty from a candidate with an added source extension', () => {
redirectModulePath.mockImplementation(filePath =>
mockRedirectModulePath.mockImplementation(filePath =>
filePath.endsWith('.fake-ext') ? false : filePath,
);
expect(
Expand All @@ -450,7 +469,7 @@ describe('redirectModulePath', () => {
"type": "empty",
}
`);
expect(redirectModulePath.mock.calls).toMatchInlineSnapshot(`
expect(mockRedirectModulePath.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"/root/smth/beep",
Expand All @@ -463,14 +482,14 @@ describe('redirectModulePath', () => {
});

test('is not called redundantly for a candidate that does exist on disk', () => {
redirectModulePath.mockImplementation(filePath => filePath);
mockRedirectModulePath.mockImplementation(filePath => filePath);
expect(Resolver.resolve(context, './bar', null)).toMatchInlineSnapshot(`
Object {
"filePath": "/root/project/bar.js",
"type": "sourceFile",
}
`);
expect(redirectModulePath.mock.calls).toMatchInlineSnapshot(`
expect(mockRedirectModulePath.mock.calls).toMatchInlineSnapshot(`
Array [
Array [
"/root/project/bar",
Expand Down
10 changes: 5 additions & 5 deletions packages/metro-resolver/src/resolve.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import InvalidPackageConfigurationError from './errors/InvalidPackageConfigurati
import InvalidPackageError from './errors/InvalidPackageError';
import PackagePathNotExportedError from './errors/PackagePathNotExportedError';
import {resolvePackageTargetFromExports} from './PackageExportsResolve';
import {getPackageEntryPoint} from './PackageResolve';
import {getPackageEntryPoint, redirectModulePath} from './PackageResolve';
import resolveAsset from './resolveAsset';
import isAssetFile from './utils/isAssetFile';
import path from 'path';
Expand Down Expand Up @@ -57,7 +57,7 @@ function resolve(
return result.resolution;
}

const realModuleName = context.redirectModulePath(moduleName);
const realModuleName = redirectModulePath(context, moduleName);

// exclude
if (realModuleName === false) {
Expand Down Expand Up @@ -155,7 +155,7 @@ function resolve(
.filter(Boolean)
.concat(extraPaths);
for (let i = 0; i < allDirPaths.length; ++i) {
const candidate = context.redirectModulePath(allDirPaths[i]);
const candidate = redirectModulePath(context, allDirPaths[i]);

if (candidate === false) {
return {type: 'empty'};
Expand Down Expand Up @@ -224,7 +224,7 @@ function resolveModulePath(
? toModuleName
: toModuleName.replaceAll('/', '\\')
: path.join(path.dirname(context.originModulePath), toModuleName);
const redirectedPath = context.redirectModulePath(modulePath);
const redirectedPath = redirectModulePath(context, modulePath);
if (redirectedPath === false) {
return resolvedAs({type: 'empty'});
}
Expand Down Expand Up @@ -565,7 +565,7 @@ function resolveSourceFileForExt(
const filePath = `${context.filePathPrefix}${extension}`;
const redirectedPath =
// Any redirections for the bare path have already happened
extension !== '' ? context.redirectModulePath(filePath) : filePath;
extension !== '' ? redirectModulePath(context, filePath) : filePath;
if (redirectedPath === false) {
return {type: 'empty'};
}
Expand Down

0 comments on commit 5e96d17

Please sign in to comment.