Fix NODE_PATH resolving for relative paths #3616
Conversation
It is failing because babel-jest traverses directories up searching for package.json, but the path is relative. It is exposing the underlying issue that NODE_PATH can be relative, but the code assumes all paths are absolute by that point.
This fixes the issue because the rest of the code assumes the path is already absolute. We also resolve symlink if necessary.
| "testEnvironment": "node", | ||
| "transform": { | ||
| "^.+\\.jsx?$": "../../packages/babel-jest" | ||
| } |
There was a problem hiding this comment.
babel-jest is the default transform, so it's not necessary to include it
There was a problem hiding this comment.
I only added because it didn't reproduce otherwise. I'm not sure why?
There was a problem hiding this comment.
Because "babel-jest" would use the version from npm, this is fine.
Generated by 🚫 dangerJS |
This test started failing, but it was not testing NODE_PATH in the first place. It just happened to include its output, and it's not empty on Travis. The test used to have some copy pasta from how we calculate nodePaths, so I just synced that copy pasta.
|
Hm, what bothers me is that |
|
Side note: it would be curious to see if this also fixes #3069 or not. |
|
It does seem to fix #3069 too. |
integration_tests/runJest.js
Outdated
| @@ -17,6 +17,7 @@ const JEST_PATH = path.resolve(__dirname, '../packages/jest-cli/bin/jest.js'); | |||
|
|
|||
| type RunJestOptions = { | |||
| skipPkgJsonCheck?: boolean, // don't complain if can't find package.json | |||
| nodePath?: string, | |||
|
This seems reasonable to me, please fix CI :) |
They are empty if NODE_PATH is empty, and split() returns [''].
|
Pushed a fix. It wasn't a timing bug, but an environmental difference. |
|
Eh, still broken. I don’t understand, there’s not enough stack and I couldn’t repro locally 😞 |
|
This is very much appreciated @gaearon. Thank you! |
* Add failing integration test for relative NODE_PATH It is failing because babel-jest traverses directories up searching for package.json, but the path is relative. It is exposing the underlying issue that NODE_PATH can be relative, but the code assumes all paths are absolute by that point. * Resolve NODE_PATH to absolute paths This fixes the issue because the rest of the code assumes the path is already absolute. We also resolve symlink if necessary. * Fix unrelated test to match the code This test started failing, but it was not testing NODE_PATH in the first place. It just happened to include its output, and it's not empty on Travis. The test used to have some copy pasta from how we calculate nodePaths, so I just synced that copy pasta. * Nitty nit * Exclude empty items from the array They are empty if NODE_PATH is empty, and split() returns ['']. * fs.realpathSync() can return undefined * Fix test to match last change
|
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. |
The downstream issue was reported in facebook/create-react-app#2230 (comment). The reproducing case is in https://github.com/ingro/cra-test-bug.
Babel Jest transform started climbing directories searching for
package.json. People who run Jest withNODE_PATH=srcand then importAppexpecting it to resolve tosrc/App.jsstarted getting this error:This is because the
filenameargument togetCacheKeyis a relative path in this case. However the function expects it to be absolute, and all other cases I logged showed absolute paths being passed. Even themodulePathsoption (which is equivalent toNODE_PATH) gets resolved to absolute paths.So to fix this, I just make
NODE_PATHhandling consistent with all other cases in the resolver. They are turned to absolute paths as early as possible. To do this, I use the same code that we’ve been using in Create React App itself for many months.