Resolve symlinked modules correctly #92
Conversation
Some systems (e.g. rush, pnpm) use symlinks to create a recursive dependency graph
instead of relying on the hoisting aspect of the node module resolution algorithm (like npm and yarn do)
in these cases, we want to resolve to the real path of a module, so that the tree structure below
only ever tries to run the `x` module once (rather than once on each module that depends on it)
- node_modules
- a
- node_modules
- symlink to x
- b
- node_modules
- symlink to x
Some systems (e.g. rush, pnpm) use symlinks to create a recursive dependency graph
instead of relying on the hoisting aspect of the node module resolution algorithm (like npm and yarn do)
in these cases, we want to resolve to the real path of a module, so that the tree structure below
only ever tries to run the `x` module once (rather than once on each module that depends on it)
- node_modules
- a
- node_modules
- symlink to x
- b
- node_modules
- symlink to x
|
CI now passes for Node 6. Current CI failures happen on older versions of node on code I didn't touch (due to ES6 syntax). |
|
@defunctzombie Ping. Is this package still maintained? |
package.json
Outdated
| @@ -8,7 +8,8 @@ | |||
| "empty.js" | |||
| ], | |||
| "scripts": { | |||
| "test": "mocha --reporter list test/*.js" | |||
| "test": "mocha --reporter list test/*.js", | |||
| "postinstall": "node scripts/setup-symlinks.js" | |||
There was a problem hiding this comment.
End users don't need this link to happen so I'd prefer if the module didn't do it for all users.
index.js
Outdated
| if (notPath) callback(err, path, pkg); | ||
| else { | ||
| fs.realpath(path, function(notReal, real) { | ||
| if (notReal) callback(err, path, pkg); |
There was a problem hiding this comment.
Please follow the code style already present in the document.
| @@ -216,6 +216,20 @@ function resolve(id, opts, cb) { | |||
| opts = opts || {}; | |||
| opts.filename = opts.filename || ''; | |||
|
|
|||
| var cb = function(err, path, pkg) { | |||
There was a problem hiding this comment.
What is this set of nested functions trying to do?
There was a problem hiding this comment.
- if it's a module name, resolve to it
- if it's something in the filesystem, then
2a) if it's a regular file, resolve to it
2b) if it's a symlink, resolve to its realpath
- run test setup in test command - follow code style
|
@defunctzombie I made the code style change and moved the test setup script to the |
|
@defunctzombie Is there anything else that needs to be done for this to be ready to merge? I suppose get tests passing in older version of node? |
|
It's fair to say that I don't maintain or keep this module updated very much. Which upstream module(s) depend on this enough to have their maintainers consider picking up this module as well? |
|
I see. Jest is currently using this package (see jestjs/jest#9511). I suppose the Jest maintainers might be amenable to taking over ownership (or at the very least forking their own version to address their needs). |
|
@rtsao I'd be happy to see this PR land if the tests were updated to pass on the versions of node that jest actively supports - I don't care if old node version support is dropped. If you want to prepare another PR building from this one and ping me I can merge it. I am also happy to have jest maintainers pickup this module or logic if that is easier for them. I do not commit resources to this module. |
|
Thanks @defunctzombie! @lhorie was kind enough to grant me push access so I've just made the changes here. As of Jest v25, Node 6.x support was removed. Additionally, Node 6.x EOL was April 30, 2019. So I've updated the travis.yml to test against 8.x, 10.x and 12.x and added an npm lockfile so that tests will be deterministic and won't break in the future. |
Some systems (e.g. rush, pnpm) use symlinks to create a recursive dependency graph
instead of relying on the hoisting aspect of the node module resolution algorithm (like npm and yarn do)
in these cases, we want to resolve to the real path of a module, so that the tree structure below
only ever tries to run the
xmodule once (rather than once on each module that depends on it)There's an example of a downstream bug here: https://github.com/lhorie/jest-browser-bug (via jestjs/jest#7840)