jestjs / jest

Delightful JavaScript Testing.
https://jestjs.io
MIT License
43.85k stars 6.39k forks source link

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

Open iamchathu opened 1 year ago

iamchathu commented 1 year ago

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
mrazauskas commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

Any updates on this?

github-actions[bot] commented 1 year ago

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.

iamchathu commented 1 year ago

Seems there is no update on this

github-actions[bot] commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

@mrazauskas I can take a crack at this

mrazauskas commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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