feat: implement the defaultAsyncResolver#15679
Conversation
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
babel-jest
babel-plugin-jest-hoist
babel-preset-jest
create-jest
@jest/diff-sequences
expect
@jest/expect-utils
jest
jest-changed-files
jest-circus
jest-cli
jest-config
@jest/console
@jest/core
@jest/create-cache-key-function
jest-diff
jest-docblock
jest-each
@jest/environment
jest-environment-jsdom
@jest/environment-jsdom-abstract
jest-environment-node
@jest/expect
@jest/fake-timers
@jest/get-type
@jest/globals
jest-haste-map
jest-jasmine2
jest-leak-detector
jest-matcher-utils
jest-message-util
jest-mock
@jest/pattern
jest-phabricator
jest-regex-util
@jest/reporters
jest-resolve
jest-resolve-dependencies
jest-runner
jest-runtime
@jest/schemas
jest-snapshot
@jest/snapshot-utils
@jest/source-map
@jest/test-result
@jest/test-sequencer
@jest/transform
@jest/types
jest-util
jest-validate
jest-watcher
jest-worker
pretty-format
commit: |
7e1a758 to
87874b1
Compare
SimenB
left a comment
There was a problem hiding this comment.
Nice!
Could you update the changelog as well?
Done! @SimenB CI is also passing now. |
bfd45e0 to
47d9b5b
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds and wires up an asynchronous version of Jest’s default module resolver across documentation, implementation, tests, snapshots, and the changelog.
- Introduces
defaultAsyncResolverAPI in the docs and types. - Refactors
defaultResolver.tsto share logic between sync and async resolver paths. - Updates
Resolverclass, tests, e2e snapshots, and the changelog to incorporate the new async resolver.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| website/versioned_docs/version-30.0/Configuration.md | Documented the new defaultAsyncResolver option |
| packages/jest-resolve/src/resolver.ts | Passed defaultAsyncResolver into sync/async flows |
| packages/jest-resolve/src/defaultResolver.ts | Extracted common logic and exported async resolver |
| packages/jest-resolve/src/tests/resolve.test.ts | Imported and asserted presence of the new resolver |
| e2e/tests/* | Updated stack-trace line numbers in snapshots |
| CHANGELOG.md | Logged the feature under [jest-resolver] |
Comments suppressed due to low confidence (3)
packages/jest-resolve/src/defaultResolver.ts:97
- Removing the absolute resolution of
basedirmay allow relative directories to slip through, potentially breaking resolution. Consider reintroducingbasedir = path.resolve(basedir);or validating thatoptions.basediris always absolute.
// make sure that `basedir` is an absolute path
packages/jest-resolve/src/tests/resolve.test.ts:16
- We should add direct tests for
defaultAsyncResolverto confirm it returns the same results asdefaultResolverwhen resolving valid paths and rejects on missing modules.
import defaultResolver, {defaultAsyncResolver} from '../defaultResolver';
website/versioned_docs/version-30.0/Configuration.md:1499
- [nitpick] Consider adding a short code snippet showing how to call
defaultAsyncResolverwithin an async custom resolver, mirroring the example fordefaultResolverabove.
Similarly, the `defaultAsyncResolver` is the default async resolver which takes the same arguments and returns a promise that resolves with a string or rejects with an error.
| }); | ||
| }; | ||
| ``` | ||
|
|
There was a problem hiding this comment.
There is another way to do similar with new resolver:
module.exports = (path, options) => {
// Call the defaultResolver, so we leverage its cache, error handling, etc.
return options.defaultResolver(path, {
...options,
// HACK!!!
// this is option from unrs-resolver from https://github.com/unrs/unrs-resolver?tab=readme-ov-file#main-field
// unrs-resolver used from jest-resolve now https://github.com/jestjs/jest/blob/v30.0.0/packages/jest-resolve/src/defaultResolver.ts#L84-L98
// We use the fact that jest-resolve just pass extra options to resolver
mainFields: ["react-native", "main"],
});
};There was a problem hiding this comment.
that seems like a very reasonable way of doing it. should be added to the docs 😀
There was a problem hiding this comment.
There was a problem hiding this comment.
As for exposing options from unrs-resolver, it seems it is not necessary, interface ResolverOptions already extends UpstreamResolveOptions.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Follow up #15619 (comment)
packageFilterhas been removed with #15619, so related document has just been updated.Test plan
cc @cpojer @SimenB