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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Manual mocks file name for node protocol imports (node:) #14040

Open
iamchathu opened this issue Mar 30, 2023 · 12 comments
Open

[Bug]: Manual mocks file name for node protocol imports (node:) #14040

iamchathu opened this issue Mar 30, 2023 · 12 comments

Comments

@iamchathu
Copy link

Version

^29.5.0

Steps to reproduce

I have added minimal code setup to following codesandbox

https://codesandbox.io/p/sandbox/jest-issue-node-protocol-b6ivxb

Expected behavior

To work with node: protocol inputs in manaul mocks without node: in the file name.

Actual behavior

I can get this working on MacOS renaming the file to node:os.ts
But does failed checkout in Windows as colon is not permited to use in filename. Also Codesandbox doesn't allows to have colons in the file.

Additional context

No response

Environment

System:
    OS: macOS 12.6.3
    CPU: (8) x64 Intel(R) Core(TM) i7-4870HQ CPU @ 2.50GHz
  Binaries:
    Node: 18.14.2 - /usr/local/bin/node
    Yarn: 1.22.19 - ~/.yarn/bin/yarn
    npm: 9.5.0 - /usr/local/bin/npm
@iamchathu iamchathu changed the title [Bug]: [Bug]: Manual mocks file name for node protocol imports (node:) Mar 30, 2023
@mrazauskas
Copy link
Contributor

mrazauskas commented Mar 30, 2023

Perhaps node: should be treated as namespaces module? I mean, mock file should be looked up in node directory, i.e. node/os.ts.

I'm trying to think how the fix should look. Does this make sense?

EDIT node: is URL scheme. It might be better to simply ignore it while resolving manual mocks. Or?

@iamchathu
Copy link
Author

I found out when you use it without node: you need to export is as module.export oppose to export default with node: version of manual mock. Is there any behaviour change on this?

@iamchathu
Copy link
Author

Any updates on this?

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label May 26, 2023
@iamchathu
Copy link
Author

Seems there is no update on this

@github-actions github-actions bot removed the Stale label May 26, 2023
@github-actions
Copy link

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

@KhaledElmorsy
Copy link
Contributor

Perhaps node: should be treated as namespaces module? I mean, mock file should be looked up in node directory, i.e. node/os.ts.

I'm trying to think how the fix should look. Does this make sense?

EDIT node: is URL scheme. It might be better to simply ignore it while resolving manual mocks. Or?

I think moving towards a separate folder is a good approach to match Node's migration to the URL scheme. This'll free up the module paths avoiding future collisions.

But given the scale of imposing a change like this, maybe it should start as an opt in feature, i.e. useNodeURLPath.

@KhaledElmorsy
Copy link
Contributor

@mrazauskas I can take a crack at this

@mrazauskas
Copy link
Contributor

Go on (;

I was playing with this a but. Here is one detail to think about: what happens if a user is importing from 'node:fs', but in the test file jest.mock('fs') is called. I think that should work, because 'node:fs' and 'fs' should point to the same module.

@KhaledElmorsy
Copy link
Contributor

KhaledElmorsy commented Jun 29, 2023

I agree, and for compatibility's sake, this should be the default behavior.

I also think that we should add an option to force paths to use the node URL scheme and /__mocks__/node to disambiguate between core modules and future packages with the same name since npm are willing to give the paths away now.

Eventually, this default behavior should be flipped where, to match Node's adoption, the expectation will be to always use the URL scheme and put manual mocks in the node folder.

What do you think?

@mrazauskas
Copy link
Contributor

It would be better to separate the fix of this issue and the /__mocks__/node path into two PRs / problems. It would be good to fix the issue without any breaking changes.

The thing is that adding new options is always tricky. Jest has too many options. I see users who are already lost and can’t find what they look. Even simple helpers like the --showConfig flag are overlooked.

@KhaledElmorsy
Copy link
Contributor

KhaledElmorsy commented Jul 4, 2023

@mrazauskas Apologies for the late reply. I agree with your point and have just finished a fix to just the issue.

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