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

absolute paths in moduleDirectories are invalid in Windows OS #5396

Closed
warren-bank opened this issue Jan 26, 2018 · 2 comments
Closed

absolute paths in moduleDirectories are invalid in Windows OS #5396

warren-bank opened this issue Jan 26, 2018 · 2 comments

Comments

@warren-bank
Copy link
Contributor

warren-bank commented Jan 26, 2018

https://github.com/facebook/jest
https://github.com/facebook/jest/tree/master/packages/jest-resolve
https://github.com/facebook/jest/blob/master/packages/jest-resolve/src/node_modules_paths.js


error:

  • absolute paths in moduleDirectories are invalid in Windows OS
  • results in CommonJS modules not being found
  • import statements throw an Error
  • all tests fail

evidence:

file:
packages/jest-resolve/src/node_modules_paths.js

line: 54

insert code:
console.log('dirs:', JSON.stringify(dirs, null, 2))


patch v1:

file:
packages/jest-resolve/src/node_modules_paths.js

line: 50

original code:
return path.join(prefix, aPath, moduleDir);

modified code:
return path.isAbsolute(moduleDir) ? moduleDir : path.join(prefix, aPath, moduleDir);

effects:

  • dirs.length will not change
  • dirs will contain paths.length duplicates of each absolute path in modules
  • absolute paths will be valid
  • absolute paths for Windows OS will no-longer be broken

patch v2:

file:
packages/jest-resolve/src/node_modules_paths.js

lines: 47 - 53

original code:

  const dirs = paths.reduce((dirs, aPath) => {
    return dirs.concat(
      modules.map(moduleDir => {
        return path.join(prefix, aPath, moduleDir);
      }),
    );
  }, []);

modified code:

  const dirs = paths
    .reduce((dirs, aPath) => {
      return dirs.concat(
        modules.map(moduleDir => {
          return path.isAbsolute(moduleDir) ? ((aPath === basedirAbs) ? moduleDir : null) : path.join(prefix, aPath, moduleDir);
        }),
      );
    }, [])
    .filter(dir => (dir !== null))
  ;

effects:

  • each absolute path in modules is only returned for a particular iteration of paths.reduce()
  • other iterations inject a null value
  • dirs is then filtered to remove these null values
  • dirs.length will be reduced, which could slightly boost performance
  • absolute paths will be valid
  • absolute paths for Windows OS will no-longer be broken

warren-bank added a commit to warren-bank/fork-jest that referenced this issue Jan 26, 2018
@warren-bank warren-bank changed the title paths in moduleDirectories are invalid under Windows OS absolute paths in moduleDirectories are invalid under Windows OS Jan 26, 2018
@warren-bank warren-bank changed the title absolute paths in moduleDirectories are invalid under Windows OS absolute paths in moduleDirectories are invalid in Windows OS Jan 26, 2018
warren-bank added a commit to warren-bank/fork-jest that referenced this issue Jan 26, 2018
@warren-bank
Copy link
Contributor Author

warren-bank commented Jan 27, 2018

For a concrete example that's fairly minimal:

git clone "https://github.com/warren-bank/react-redux-bindings.git"
cd "react-redux-bindings"
npm install
cd "tests"
npm install
npm run test

Background:

  • I wrote and tested this project in Linux.. not so long ago.. no problem
  • I was laying on the sofa this evening
    • watching tv
    • browsing the web from Windows (ie: my hanging-around OS of choice)
    • re-reading some of the code in this project
    • felt the need to push a super minor update
    • ran the unit tests.. and all failed
    • "wtf?".. I ask
    • "did jest make a recent update that conflicts with my configuration?".. I ask
    • knowing that Windows generally sucks for development..
      • rebooted to Linux
      • ran the above set of commands
      • all tests pass
    • rebooted back to Windows
    • traced the problem (in jest)
    • figured out what's breaking and how to fix it
    • forked jest and submitted a pull request
    • made an issue in the tracker to be more verbose about the problem
    • and.. here we are :)

warren-bank added a commit to warren-bank/fork-jest that referenced this issue Jan 28, 2018
@cpojer cpojer closed this as completed in 1f6e35c Jan 29, 2018
@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 May 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant