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

working directory makes require.resolve fail with custom paths on v8.x #18686

Closed
FND opened this issue Feb 9, 2018 · 7 comments
Closed

working directory makes require.resolve fail with custom paths on v8.x #18686

FND opened this issue Feb 9, 2018 · 7 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@FND
Copy link

FND commented Feb 9, 2018

  • Version: v8.9.4
  • Platform: Darwin 17.4.0
  • Subsystem: modules

Invoking require.resolve with the paths option fails if the respective directory does not match the current working directory. This appears to have been fixed in v9.x, but continues to affect LTS versions.

Reduced test case below - I hope these Bash one-liners are readily understandable, otherwise please let me know:

$ cd /tmp

# setup: create sample project with dummy package
$ mkdir -p test_case/node_modules/dummy
$ echo 'console.log("…");' > test_case/node_modules/dummy/index.js

# variant A [FAILS]: working directory does not match `require.resolve.paths`
$ pwd
/tmp
$ node -e 'console.log(require.resolve("dummy/index.js", { paths: ["/tmp/test_case"] }))'
Error: Cannot find module 'dummy/index.js'

# variant B [SUCCEEDS]: working directory and `require.resolve.paths` are identical
$ cd test_case
$ pwd
/tmp/test_case
$ node -e 'console.log(require.resolve("dummy/index.js", { paths: ["/tmp/test_case"] }))'
/tmp/test_case/node_modules/dummy/index.js

Setting process.chdir("/tmp/test_case") before invoking require.resolve does not make any difference.

On Node v9.5.0, both variants work as expected (i.e. just like variant B).

@cjihrig
Copy link
Contributor

cjihrig commented Feb 9, 2018

If it works in Node 9, you probably are looking for the changes in 8578e81. It looks like it has been backported to the v8.x-staging branch. However, I don't see it in the upcoming 8.10.0 release proposal.

@FND
Copy link
Author

FND commented Feb 10, 2018

Thanks @cjihrig; I'd looked through the changelog, but wasn't sure whether that commit was relevant.

Is there a way I could confirm this fix by locally patching Node? I've extracted the v8.9.4 tarball, but can't find any relevant .js files within. (I not familiar with Node's internals.)

Also, should I pipe up in #18336, requesting inclusion after confirming the above? (I hesitate to just barge in there because I'm equally unfamiliar with Node's community processes.)

@bnoordhuis
Copy link
Member

@FND Did you download the binary tarball? You'll need the source tarball, JS files are in lib/.

You can apply the patch by tacking on .patch (or .diff) to the commit URL Colin posted, i.e., https://github.com/nodejs/node/commit/8578e81b2b41eac6aea7a812b8e5f35a7362a6df.patch

@FND
Copy link
Author

FND commented Feb 10, 2018

@bnoordhuis: You're right, thanks - I'd only checked the binary tarball because I had assumed 🤦‍♂️ mere JavaScript files didn't require compiling Node from source.

After applying 8578e81, my test case above passes with the patched v8.9.4 binary - so @cjihrig was indeed correct.

What's the next step for getting this backported for an LTS release?

@bnoordhuis
Copy link
Member

Let's ask. @gibfahn Is 8578e81 on track for the next v8.x?

FND added a commit to faucet-pipeline/faucet-pipeline-core that referenced this issue Feb 15, 2018
note that path resolution for third-party packages currently fails on
Node v8 if CWD is not the same as `referenceDir`:
nodejs/node#18686
@FND FND mentioned this issue Feb 15, 2018
@gibfahn
Copy link
Member

gibfahn commented Feb 19, 2018

Let's ask. @gibfahn Is 8578e81 on track for the next v8.x?

Yes.

FND added a commit to faucet-pipeline/faucet-pipeline-core that referenced this issue Feb 19, 2018
note that path resolution for third-party packages currently fails on
Node v8 if CWD is not the same as `referenceDir`:
nodejs/node#18686
FND added a commit to faucet-pipeline/faucet-pipeline-core that referenced this issue Feb 19, 2018
note that path resolution for third-party packages currently fails on
Node v8 if CWD is not the same as `referenceDir`:
nodejs/node#18686
@joyeecheung joyeecheung added module Issues and PRs related to the module subsystem. v8.x labels Feb 20, 2018
@richardlau
Copy link
Member

8578e81 was backported to 8.10.0 as aaca447333.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module Issues and PRs related to the module subsystem.
Projects
None yet
Development

No branches or pull requests

6 participants