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

path.resolve is broken with different copy of graceful-fs #9923

Closed
rix0rrr opened this issue Apr 29, 2020 · 6 comments
Closed

path.resolve is broken with different copy of graceful-fs #9923

rix0rrr opened this issue Apr 29, 2020 · 6 comments

Comments

@rix0rrr
Copy link

rix0rrr commented Apr 29, 2020

馃悰 Bug Report

An issue has been introduced by Jest 25.5.0 depending on graceful-fs. 25.5.1 does not fix it.

  • If there are multiple copies of graceful-fs in the dependency tree (can happen if you depend on a dependency which has an older version of graceful-fs than jest itself now does)
  • And in a test you do process.chdir()
  • Followed by path.resolve()
  • Then the process.cwd() that path.resolve() uses, uses a different copy of graceful-fs, which has a different copy of its cached value for cwd, and so path.resolve calculates the path relative to the wrong directory.

To Reproduce

test('something', () => {
  process.chdir('/somewhere');
  path.resolve('.')
  // Should be returning '/somewhere'
  // Actually returns '/process/starting/directory'
});

Check this out, run:

$ yarn install
$ yarn test

Expected behavior

process.chdir(A);
path.resolve('.') == A

Link to repl or repo (highly encouraged)

https://gist.github.com/rix0rrr/c62608a504987fc63d95b789a78a529a

envinfo


  System:
    OS: macOS Mojave 10.14.6
    CPU: (12) x64 Intel(R) Core(TM) i7-8750H CPU @ 2.20GHz
  Binaries:
    Node: 12.12.0 - /usr/local/bin/node
    Yarn: 1.19.1 - /usr/local/bin/yarn
    npm: 6.14.4 - /usr/local/bin/npm
  npmPackages:
    jest: ^25.5.0 => 25.5.1 
@SimenB
Copy link
Member

SimenB commented Apr 29, 2020

Thanks for the detailed bug report!

Jest has depended on graceful-fs for years, but via gracefulify, 25.5 only made the dependency explicit rather than rely on monkey patching. I didn't even know graceful-fs cached and overrode both process.chdir and process.cwd, that seems horrible to me. git blame says it's years and years old though...

I don't really have a solution I'm afraid. yarn resolutions, perhaps? Should allow you to force a version throughout your dependency tree. If you don't use yarn, npm will also get resolutions in v7, but that doesn't help you now. patch-package?


As an aside, Jest has never really supported process.chdir, it breaks a bunch of assumptions we make about the structure of the project.

@rix0rrr
Copy link
Author

rix0rrr commented Apr 30, 2020

Thanks for the thoughtful response @SimenB. TIL about yarn resolutions, and also was not aware that we weren't supposed to use process.chdir.

We'll hack around it with nasty dependency management trickery, and fortunately Jest is only a dev dependency and not a prod dependency, but this seems like a sharp edge you might want to do something about.

As an aside, if you really don't support process.chdir(), can you loudly yell at me when I try to do it anyway (monkey patch process.chdir() to throw an error for example), rather than introducing subtle failure conditions then? (Or, preferably, move to support it of course 馃樃 )

I'd rather would be told immediately while trying to write my test suite that what I'm doing cannot work, rather than having to now go back and make sure I don't accidentally chdir in all of my existing code.

@SimenB
Copy link
Member

SimenB commented Apr 30, 2020

Yeah, makes sense. We already override process.exit since it's a weird thing to call in tests, could at least print a warning about chdir.
I think ideally we'd add support for it in an isolated manner, so a chdir in one test just updated process.cwd() for that one test without actually having a side effect for other tests. We wouldn't be able to force that for e.g. path and fs operations though as node core modules are global, so might be better to just yell and say these are unchartered waters than some half-working solution with specific caveats

@github-actions
Copy link

This issue is stale because it has been open for 1 year 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 Feb 17, 2023
@github-actions
Copy link

This issue was closed because it has been stalled for 30 days with no activity. Please open a new issue if the issue is still relevant, linking to this one.

@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Mar 19, 2023
@github-actions
Copy link

This issue 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 Apr 22, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants