Mock node modules with node: URL scheme

[KhaledElmorsy]

Summary

Allow modules that use node: paths, i.e. node:fs, to be mocked and mapped to manual mocks appropriately.

With Node adopting the aforementioned URL scheme, more users will migrate to it and expect their tools to support it.

Jest already incorporates checks for the URL scheme but not for mapping mocks and module names.

With this change, Jest strips the URL when resolving mocks and assigning moduleIDs, making any of the following test setups in the next section valid and point to the same module.

Source issue

Test plan

The change resolves the URL to the module name, so the following 3 e2e tests were added to verify that by checking if the fs module was mocked in the following 3 scenarios.

Realistically, the first scenario is the target use case.

// URL scheme for imports and mocks
const fs = require('node:fs')
jest.mock('node:fs')

// URL scheme only when mocking
const fs = require('fs')
jest.mock('node:fs')

// URL scheme only when importing
const fs = require('node:fs')
jest.mock('fs')

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: KhaledElmorsy / name: Khaled Elmorsy (6a11f1e, 630d01a)

[netlify]

✅ Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	630d01a
🔍 Latest deploy log	https://app.netlify.com/sites/jestjs/deploys/64a6e82f6ebee20008db2610
😎 Deploy Preview	https://deploy-preview-14297--jestjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

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

[mrazauskas]

Did you check if jest.unmock() works? It uses resolver’s getModuleID() method and this one is not touched in your patch. That’s why I’m asking.

I found a branch where I was play with this fix. Seemed like it was enough to strip 'node:' before running the logic of getMockModule(), getMockModuleAsync(), getModuleID() and getModuleIDAsync() methods. I had no time to write proper tests, so it can be that I missed something. Just wondering why the changes you made are so complex? It felt like the current logic should handle everything if 'node:' is removed.

[KhaledElmorsy]

Did you check if jest.unmock() works? It uses resolver’s getModuleID() method and this one is not touched in your patch. That’s why I’m asking.

I found a branch where I was play with this fix. Seemed like it was enough to strip 'node:' before running the logic of getMockModule(), getMockModuleAsync(), getModuleID() and getModuleIDAsync() methods. I had no time to write proper tests, so it can be that I missed something. Just wondering why the changes you made are so complex? It felt like the current logic should handle everything if 'node:' is removed.

I strip node: in Resolver._getAbsolutePath which getModuleID uses and has a branch for core modules, so it felt right to extend the processing in that branch to generalize it to other methods that could use _getAbsolutePath.

The two spots where I strip node: are _getAbsolutePath and in Runtime.requireMock before the moduleName gets passed to getMockModule.

The last change was editing getMockModule(Async) to always return either mocks or null, because the current logic, after not finding a mock with the passed module name returns either the original module name or the result of passing the original module name into resolveStubModuleName. But those two values aren't mocks and force the methods that consume getMockModule to get really convoluted to know what getMockModule returned.

[KhaledElmorsy]

@mrazauskas I hope I answered your questions well enough. Is there anything more I should do on this PR?

[mrazauskas]

Thanks for the details and for reminder.

I was trying to look from the distance and to think a bit. These lines made me think:

jest/packages/jest-runtime/src/index.ts

Lines 1683 to 1687 in 70b17d2

	private _requireCoreModule(moduleName: string, supportPrefix: boolean) {
	const moduleWithoutNodePrefix =
	supportPrefix && moduleName.startsWith('node:')
	? moduleName.slice('node:'.length)
	: moduleName;

Now strings are mostly normalized in jest-resolve, but sometimes this will happen in jest-runtime. Also it can happen in higher or in lower layer. Hm.. The code works, of course. But is jest-resolve or jest-runtime broken? In general it seems like they are not. They are just tricked by user input.

What if we would normalize the user input as the first thing at all the entry spots? That would ensure that we pass clean up data to modules which already do the right job.

[mrazauskas]

By the way, you don’t have to force push. Commits are squashed when PRs are merged.

[KhaledElmorsy]

I definitely see what you mean in doing it in one place and that would be definite improvement to the codebase. There's a few different methods that take a path as input that would need to changed, like jest.mock, jest.unmock, etc. Some pass it directly to Resolver.getModuleID which normalizes and others like Runtime.requireMockOrModule send it in many different directions. Both would benefit from pre-normalization.

Having the normalization in only the upper Runtime would mean updating those method that pass directly to the resolver and removing its normalization logic. It's doable and should be done but it'll be a lot of changes and needs a good look.

I was actually meaning to replace the snippet you linked with Resolver. normalizeCoreModuleSpecifier and I'm kicking myself for forgetting (good catch).

I think we should push this fix as is, after replacing that snippet, and then implement the more centralized and widely used normalizer in Runtime in a separate PR. Maybe it can have its own open issue so that anyone can jump on that major refactor, if I can't dedicate the time for it.

What do you think?

[KhaledElmorsy]

Looks like I didn't forget 😆

6a11f1e#diff-c0d5b59e96fdc7ffc98405e8afb46d525505bc7b1c24916b5c8482de5a186c00L1707-L1712

[mrazauskas]

You did not forget. This is why I saw these lines. jest-runtime was build before the node: scheme. Some logic is tweaked a bit, but in general it does not support the node: scheme.

Why not to implement the support in one go? Not sure why do you see this as major refactor? Just a line should be added for each method which take module name string. What is "centralized and widely used normalizer"? Is this those five lines of normalizeCoreModuleSpecifier() method or you have plans to normalize something more?

That was just an idea. Feel free rethink it or ignore it. Also worth mentioning that I don’t have rights to merge PRs. I can only leave comments here and there (;

[KhaledElmorsy]

I was thinking of potential normalization for other cases than node:, but I agree, it's probably just that for now.

I gave this some more thought and I think both approaches have merits. If a dozen runtime methods pass to the resolver which normalizes in only two methods with only the snippet method being the outlier in the Runtime, then it's also clean to consolidate it in the resolver like it is. Both approaches are viable, but I'm okay with leaving it is as is.

Also, I really appreciate your comments and insights, it's always good to be met with different approaches to consider and apply.

[KhaledElmorsy]

@SimenB could you please approve/merge this PR?

[mrazauskas]

By the way, what happens if jest.mock('node:fs') is called on Node.js v14.15 (the lowest supported by Jest)? Calling require('node:fs') throws an error.

[mrazauskas]

I was experimenting with somewhat bigger idea to add support for URL strings and URL objects. That would work for node: and file: schemas. The change is rather minimal. Curious what you think? See #14332

[KhaledElmorsy]

While I haven't seen URLs being used to import modules, if it works, then there's no reason not to support it fully.

I you should pause until this PR is merged because of the refactors & corrections, and then expand the resolution logic it adds to account for URL's as you did. I personally feel like these additions could have been added to this PR given how similar the approaches are.

But still, it's good to think of more general applications and consider the bigger picture, as well finding the most direct way to address issues.

[thedavidprice]

Hi @KhaledElmorsy @mrazauskas We're blocked on an ESM transition due to #14040 If we come up with a patch, would you like us to try and coordinate between this PR and #14332 or simply proceed independently?

cc @virtuoushub

[mrazauskas]

Not sure I follow. How is the node: prefix related with ESM? Also could you elaborate what is exactly blocking you? There is some inconvenience, but that does not look blocking for me.

[virtuoushub]

Not sure I follow. How is the node: prefix related with ESM? Also could you elaborate what is exactly blocking you? There is some inconvenience, but that does not look blocking for me.

Not directly related to ESM, but as a precursor to ESM work, I recently opened a PR in redwood to prefer the node protocol. That PR is what is currently blocked.

When importing builtin modules, it's better to use the node: protocol as it makes it perfectly clear that the package is a Node.js builtin module.

see also: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-node-protocol.md

[tdeekens]

I am not sure if this relates but I am running into Cannot find module '/[...]/babel.config.js' in Jest test runs. This only happens after the only change is that I refactored a medium size codebase to use the node: protocol.

[mrazauskas]

@tdeekens Hard to say. I don’t think manual mocks need babel.config.js. That can be something else. Try creating minimal reproduction repo, perhaps?

[github-actions]

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.

[Tobbe]

What's needed to move this PR along?

[github-actions]

This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days.