Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(resolver): support node: prefix when loading core modules #11331

Merged
merged 6 commits into from Aug 27, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 22, 2021

Summary

Ref nodejs/node#37246 (comment)

I haven't bothered to add any node version guard. So even though only v16 supports it in require, Jest will support it across all runtimes. I think that's fine. Keeping track of all the changes to node modules is doing my head in 馃槄

Closes #11638
Closes #11686

Test plan

Added unit test

Copy link
Collaborator

@thymikee thymikee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤馃徏

@SimenB SimenB linked an issue Apr 25, 2021 that may be closed by this pull request
@SimenB
Copy link
Member Author

SimenB commented Apr 25, 2021

We should probably throw the correct error if node: is used for non-code modules as well.

@dnalborczyk
Copy link

dnalborczyk commented Apr 29, 2021

is this going into the 26.x release or into 27.x?

@SimenB
Copy link
Member Author

SimenB commented Apr 30, 2021

27

@SimenB
Copy link
Member Author

SimenB commented May 25, 2021

I forgot it before the release, though 馃榾 Will try to find the time soonish to finish this

@SimenB SimenB modified the milestones: Jest 27, High priority future May 25, 2021
@cpojer
Copy link
Member

cpojer commented May 29, 2021

I'll review once you rebase this.

@theoludwig
Copy link
Contributor

Oops, only find out now that there is already a PR for this.
There are 2 related PRs, to fix the same thing:

There is also this issue #11637

It is all duplicates of this PR, but it is implemented differently.
At least you have the choice of the implementation, once jest support node: prefix, don't forget to close the appropriate PRs and issues.

@antongolub
Copy link
Contributor

@SimenB, @cpojer,

More and more Sindre's utils (globby, cpy, etc) are moving to node:* notation. Updating these dependencies in projects breaks any jest-based tests. This is becoming a real problem. Please, give the green light to the patch asap.

@@ -1327,15 +1327,19 @@ export default class Runtime {
}

private _requireCoreModule(moduleName: string) {
if (moduleName === 'process') {
const moduleWithoutNodePrefix = moduleName.startsWith('node:')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may do not strip the prefix if the runtime supports it:

process.version.slice(1, 3) < 16
semver.lt(process.version, 'v16')

ref: https://nodejs.org/api/modules.html#modules_core_modules

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@theoludwig
Copy link
Contributor

Possible to make the tests pass again and merge that ASAP?
Really want that, there are 2 similar PRs with the tests passing and it's still taking so much time to get this feature/fix.
My CI is currently failing because of this, don't want to undo the refactor with node: syntax in my imports.

Thanks for your work! 馃槃

@SimenB SimenB merged commit 554d7d2 into jestjs:master Aug 27, 2021
@SimenB SimenB deleted the node-core-prefix branch August 27, 2021 09:51
@SimenB
Copy link
Member Author

SimenB commented Aug 27, 2021

https://github.com/facebook/jest/releases/tag/v27.1.0

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ESM and node: protocol in imports
7 participants