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

require.resolve regression in Node v12 with paths #29139

Closed
FND opened this issue Aug 15, 2019 · 2 comments
Closed

require.resolve regression in Node v12 with paths #29139

FND opened this issue Aug 15, 2019 · 2 comments

Comments

@FND
Copy link

FND commented Aug 15, 2019

  • Version: 12.8.0
  • Platform: Darwin v17.7.0 x86_64
  • Subsystem: modules (require.resolve)

related issue: #27794 - but that was fixed in v12.3.0

Given the following directory structure:

/path/to/lib
├── dummy
│   ├── index.js
│   └── src.js
└── node_modules
    └── dummy
        ├── index.js
        └── pkg.js

On Node v8, v10 and v11, the following returned /path/to/lib/dummy/src.js, but Node v12 (all minor/patch releases) throws a MODULE_NOT_FOUND exception ("Cannot find module 'dummy/src.js'"):

require.resolve("dummy/src.js", { paths: ["/path/to/lib"] });

(FWIW, I've automated bisecting Node versions: https://gist.github.com/FND/d35c5a5c128a7d0e9131ece904f601a9)

Node v12.8.0's stack trace looks interesting:

Require stack:
- /tmp/nodejs_versions/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:713:15)
    at Function.resolve (internal/modules/cjs/helpers.js:74:19)
    at [eval]:1:9
    at Script.runInThisContext (vm.js:126:20)
    at Object.runInThisContext (vm.js:316:38)
    at Object.<anonymous> ([eval]-wrapper:9:26)
    at Module._compile (internal/modules/cjs/loader.js:868:30)
    at evalScript (internal/process/execution.js:80:25)
    at internal/main/eval_string.js:23:3 {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/tmp/nodejs_versions/[eval]' ]
}

That [eval] bit seems suspect?

Arguably v12's behavior is more correct (not sure why we expected /path/to/lib/dummy to be supported in the first place), but it's a behavioral change nonetheless. Since I couldn't find any relevant entries in Node's change log, I'm not sure whether this change was intentional.

@cjihrig
Copy link
Contributor

cjihrig commented Aug 15, 2019

That [eval] bit seems suspect?

I believe that's because your test script is launching node with -e to eval command line code.

Arguably v12's behavior is more correct (not sure why we expected /path/to/lib/dummy to be supported in the first place), but it's a behavioral change nonetheless. Since I couldn't find any relevant entries in Node's change log, I'm not sure whether this change was intentional.

There was a bug that was fixed in the 12.0.0 release via #23683. I think that's what you're bumping into.

@FND
Copy link
Author

FND commented Aug 15, 2019

That all makes sense, thank you for the quick response!

@FND FND closed this as completed Aug 15, 2019
FND added a commit to faucet-pipeline/faucet-pipeline-core that referenced this issue Aug 16, 2019
`require.resolve` behavior was changed in Node v12 to comply with Node
resolution algorithm (see nodejs/node#29139) -
not sure why we thought this behavior was desirable in the first place
(introduced in 3142f2e - probably
cargo-culting `require.resolve`'s observed behavior)

however, on legacy Node versions behavior appears unchanged if the
current working directory is the same as `rootDir` - I didn't bother
enforcing modern behavior because legacy isn't worth the effort

in the process, fixed legacy detection
FND added a commit to faucet-pipeline/faucet-pipeline-core that referenced this issue Sep 2, 2019
`require.resolve` behavior was changed in Node v12 to comply with Node
resolution algorithm (see nodejs/node#29139) -
not sure why we thought this behavior was desirable in the first place
(introduced in 3142f2e - probably
cargo-culting `require.resolve`'s observed behavior)

however, on legacy Node versions behavior appears unchanged if the
current working directory is the same as `rootDir` - I didn't bother
enforcing modern behavior because legacy isn't worth the effort

in the process, fixed legacy detection
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants