Skip to content

chore: migrate resolve and resolve.exports to unrs-resolver #15619

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

JounQin
Copy link
Contributor

@JounQin JounQin commented May 24, 2025

Summary

close #15600

Test plan

There are already related test cases.

Copy link

netlify bot commented May 24, 2025

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 45df461
🔍 Latest deploy log https://app.netlify.com/projects/jestjs/deploys/6832e11c030a710008b3a97f
😎 Deploy Preview https://deploy-preview-15619--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@JounQin JounQin force-pushed the dep/unrs-resolver branch from d892bb9 to c7d496e Compare May 24, 2025 02:30
@JounQin
Copy link
Contributor Author

JounQin commented May 24, 2025

@SimenB Should we drop paths/packageFilter/pathFilter support? Otherwise we'll still need resolve as fallback.

@JounQin JounQin force-pushed the dep/unrs-resolver branch 3 times, most recently from 7bb7369 to 0b753f5 Compare May 24, 2025 02:44
@SimenB
Copy link
Member

SimenB commented May 24, 2025

@SimenB Should we drop paths/packageFilter/pathFilter support? Otherwise we'll still need resolve as fallback.

That seems fine to me. But paths is also a core require feature, surprised the resolver doesn't support it? https://nodejs.org/api/modules.html#requireresolverequest-options

I'm also wondering if no rootDir might bite us 🤔


Mind updating the docs as well?

jest/docs/Configuration.md

Lines 1469 to 1546 in 4556297

The options object provided to resolvers has the shape:
```ts
type ResolverOptions = {
/** Directory to begin resolving from. */
basedir: string;
/** List of export conditions. */
conditions?: Array<string>;
/** Instance of default resolver. */
defaultResolver: (path: string, options: ResolverOptions) => string;
/** List of file extensions to search in order. */
extensions?: Array<string>;
/** List of directory names to be looked up for modules recursively. */
moduleDirectory?: Array<string>;
/** List of `require.paths` to use if nothing is found in `node_modules`. */
paths?: Array<string>;
/** Allows transforming parsed `package.json` contents. */
packageFilter?: (pkg: PackageJSON, file: string, dir: string) => PackageJSON;
/** Allows transforms a path within a package. */
pathFilter?: (pkg: PackageJSON, path: string, relativePath: string) => string;
/** Current root directory. */
rootDir?: string;
};
```
:::tip
The `defaultResolver` passed as an option is the Jest default resolver which might be useful when you write your custom one. It takes the same arguments as your custom synchronous one, e.g. `(path, options)` and returns a string or throws.
:::
For example, if you want to respect Browserify's [`"browser"` field](https://github.com/browserify/browserify-handbook/blob/master/readme.markdown#browser-field), you can use the following resolver:
```js title="resolver.js"
const browserResolve = require('browser-resolve');
module.exports = browserResolve.sync;
```
And add it to Jest configuration:
```js tab
/** @type {import('jest').Config} */
const config = {
resolver: '<rootDir>/resolver.js',
};
module.exports = config;
```
```ts tab
import type {Config} from 'jest';
const config: Config = {
resolver: '<rootDir>/resolver.js',
};
export default config;
```
By combining `defaultResolver` and `packageFilter` we can implement a `package.json` "pre-processor" that allows us to change how the default resolver will resolve modules. For example, imagine we want to use the field `"module"` if it is present, otherwise fallback to `"main"`:
```js
module.exports = (path, options) => {
// Call the defaultResolver, so we leverage its cache, error handling, etc.
return options.defaultResolver(path, {
...options,
// Use packageFilter to process parsed `package.json` before the resolution (see https://www.npmjs.com/package/resolve#resolveid-opts-cb)
packageFilter: pkg => {
return {
...pkg,
// Alter the value of `main` before resolving the package
main: pkg.module || pkg.main,
};
},
});
};
```

@JounQin
Copy link
Contributor Author

JounQin commented May 24, 2025

@SimenB

That seems fine to me. But paths is also a core require feature, surprised the resolver doesn't support it?

I can try to help adding it a new feature for unrs-resolver if that's really really desired, but even resolve discourages using it:

opts.paths - require.paths array to use if nothing is found on the normal node_modules recursive walk (probably don't use this)

unrs-resolver/oxc-resolver is port of enhanced-resolve which doesn't support paths.

Or is there any difference between modules vs paths? We can try paths as modules when modules fails to resolve.


I'm also wondering if no rootDir might bite us 🤔

There is no rootDir option used at all, I've confirmed with resolve and resolve.exports packages. So I just removed it.

Mind updating the docs as well?

Sure after codes accepted.

@JounQin
Copy link
Contributor Author

JounQin commented May 24, 2025

I'll add paths support back by fallback to modules as I proposed above.

@cpojer
Copy link
Member

cpojer commented May 24, 2025

Wow, awesome, thanks for working on this @JounQin.

I assume this should have a noticeably positive perf impact on Jest. Would you be able to share the performance before and after this change of running Jets's own test suite? (First yarn build then yarn jest --maxWorkers=1. Because of babel caching, you probably need to run it at least once to prime the cache, and then each subsequent run can be used to measure performance of the resolvers. Then apply your changes, run yarn build, yarn jest --maxWorkers=1 again to prime the cache, and compare to the previous runs.

Note that benchmarking should also use --maxWorkers=1. Using more workers affects performance too much unless you can control what your operating system runs while benchmarking.

@JounQin JounQin force-pushed the dep/unrs-resolver branch from 0b753f5 to 3d9595c Compare May 25, 2025 07:58
@JounQin JounQin force-pushed the dep/unrs-resolver branch from 3d9595c to 45df461 Compare May 25, 2025 09:21
@JounQin
Copy link
Contributor Author

JounQin commented May 25, 2025

There are still a few test cases failing, I'll check verify if can it be fixed in unrs-resolver.

@JounQin
Copy link
Contributor Author

JounQin commented May 25, 2025

For performance benchmarking, before: 7m18s, after: 6m31s.

  System:
    OS: macOS 15.5
    CPU: (10) arm64 Apple M4
    Memory: 777.91 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Deps]: migrate resolve and resolve.exports to unrs-resolver
3 participants