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

Node 12: require.resolve(relativePath, { paths: [ '.' ] }) throws without ./ #27583

Closed
75lb opened this issue May 6, 2019 · 11 comments · Fixed by #27598 or MechanicalRock/tech-radar#14 · May be fixed by MechanicalRock/cdk-constructs#5, MechanicalRock/cdk-constructs#6 or MechanicalRock/cdk-constructs#7
Labels
module Issues and PRs related to the module subsystem.

Comments

@75lb
Copy link

75lb commented May 6, 2019

  • Version: v12.1.0
  • Platform: Darwin mba4.local 18.5.0 Darwin Kernel Version 18.5.0
  • Subsystem: Modules

I have a module lib/something.js which looks like this:

module.exports = function something () {}

In the node v10 REPL, this require.resolve invocation works fine:

> require.resolve('lib/something.js', { paths: [ '.' ] })
'/Users/lloyd/Documents/75lb/load-module/lib/something.js'

In node v12, it no longer works.

> require.resolve('lib/something.js', { paths: [ '.' ] })
Thrown:
Error: Cannot find module 'lib/something.js'
Require stack:
- /Users/lloyd/Documents/75lb/load-module/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:610:15)
    at Function.resolve (internal/modules/cjs/helpers.js:21:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/lloyd/Documents/75lb/load-module/[eval]' ]
}

In node v12, you need to prepend ./ to the module ID like this:

> require.resolve('./lib/something.js', { paths: [ '.' ] })
'/Users/lloyd/Documents/75lb/load-module/lib/something.js'

This is an issue for my users because the path passed to require.resolve comes from command-line input, e.g.

$ example lib/something.js

Is this the expected new behaviour of require.resolve or a bug? Do I need to update my tools to prepend ./?

@addaleax addaleax added the module Issues and PRs related to the module subsystem. label May 6, 2019
@Trott
Copy link
Member

Trott commented May 6, 2019

#23683 seems like a likely candidate. It seems intentional.
/ping @cjihrig

@cjihrig
Copy link
Contributor

cjihrig commented May 6, 2019

This was reported to me by @devinivy in a separate channel. The change was intentional. I believe I overlooked the case of relative paths though, which would make this a bug.

EDIT: After reviewing this report I think it's working as expected.

@75lb
Copy link
Author

75lb commented May 7, 2019

The node v12 require.resolve docs are identical to the node v10 docs yet there has been a breaking change in behaviour.

Create a lib/index.js file looking like this:

module.exports = function something () {}

Then create this reproduction script issue-27583.js:

const path = require('path')

function log (request, paths) {
  try {
    console.log(require.resolve(request, { paths }))
  } catch (err) {
    console.log('Failed:', request, paths)
  }
}

log('lib/index.js', [ '.' ])
log('lib', [ '.' ])
log('lib/index.js', [ process.cwd() ])
log('lib', [ process.cwd() ])
log('index.js', [ 'lib' ])
log('index.js', [ './lib' ])
log('index.js', [ path.resolve('./lib') ])

Each of these repro invocations fail (with "cannot find module") in node v12 yet pass in node v10. Node v10 output on my laptop:

$ node issue-27583.js
/Users/lloyd/Documents/tmp/lib/index.js
/Users/lloyd/Documents/tmp/lib/index.js
/Users/lloyd/Documents/tmp/lib/index.js
/Users/lloyd/Documents/tmp/lib/index.js
/Users/lloyd/Documents/tmp/lib/index.js
/Users/lloyd/Documents/tmp/lib/index.js
/Users/lloyd/Documents/tmp/lib/index.js

Node v12 output:

$ node issue-27583.js
Failed: lib/index.js [ '.' ]
Failed: lib [ '.' ]
Failed: lib/index.js [ '/Users/lloyd/Documents/tmp' ]
Failed: lib [ '/Users/lloyd/Documents/tmp' ]
Failed: index.js [ 'lib' ]
Failed: index.js [ './lib' ]
Failed: index.js [ '/Users/lloyd/Documents/tmp/lib' ]

@devinivy
Copy link

devinivy commented May 7, 2019

I believe there is a bug here, but it is also my impression that the docs haven't changed because in node v12 the intent was to align require.resolve()'s behavior with the current documentation. Specifically in the case that the request does not begin with './' or '/' or '../', require.resolve() should not search within the listed paths— it should only begin the module resolution algorithm from those paths. In node v10 it did search within the listed paths, and the intention was to remove that behavior in v12.

All of those examples you provide above include requests that do not begin with './' or '/' or '../', so by the module resolution algorithm they're actually being looked for inside node_modules folders relative to the paths you've listed.

@cjihrig
Copy link
Contributor

cjihrig commented May 7, 2019

The idea behind this feature is that for each item in paths, it should behave like a chdir to that directory, followed by a require() call. Given a directory lib with an index.js file inside of it, I would expect require.resolve('lib/index.js', { paths: ['.'] }) to throw. Since the path is ., there would be no chdir operation, and require('lib') and require('lib/index.js') both throw.

@75lb
Copy link
Author

75lb commented May 7, 2019

they're actually being looked for inside node_modules folders relative to the paths you've listed.

In which case I think I'll step away from using require.resolve because I need to supply custom search paths (not node_modules). In the case of my app, users supply either

  1. a path to a local plugin or
  2. the name of a plugin distributed within the app's npm package, e.g.:
$ example --plugin plugins/something.js # user-defined local plugin
$ example --plugin builtin-plugin       # a known plugin distributed with the app

Currently, I use require.resolve to search for the plugin in both the user's working directory and the running application's npm package:

const modulePath = require.resolve(pluginNameOrPath, { 
  paths: [ 
    '.', 
    path.resolve(__dirname, '..', 'plugins')
  ] 
})

If require.resolve no longer searches the paths provided, but instead searches node_modules within the paths provided then it's no longer suitable for local, arbitrary paths.

it should behave like a chdir to that directory, followed by a require() call.

In which case I'd expect at least one of the following examples to work in node v12, which they don't (notice i added the ./):

log('./index.js', [ 'lib' ])
log('./index.js', [ './lib' ])
log('./index.js', [ path.resolve('./lib') ])

@cjihrig
Copy link
Contributor

cjihrig commented May 7, 2019

Yes, require.resolve('./index.js', { paths: ['./lib'] }) should work. I think that's the bug here.

@devinivy
Copy link

devinivy commented May 7, 2019

I agree, that's exactly the bug we're dealing with here.

@cjihrig
Copy link
Contributor

cjihrig commented May 7, 2019

Proposed fix in #27598.

In which case I think I'll step away from using require.resolve because I need to supply custom search paths

With the fix in #27598, could you add a ./ to the beginning of your paths?

@75lb
Copy link
Author

75lb commented May 8, 2019

could you add a ./ to the beginning of your paths?

Not really. Say the input request is 'lodash/something.js' (search the lodash package for something.js). If I add ./ it changes the request completely ('./lodash/something.js' searches a local folder for something.js).

I'm testing your #27598 patch locally. I created two projects "one" and "two". In "one" i installed lodash, project "two" is empty. Directory structure looks like this:

$ tree -I '' -L 3
.
├── one
│   ├── node_modules
│   │   └── lodash
│   ├── package-lock.json
│   └── package.json
└── two
    └── package.json

For a moment I was worried because, after a cd to the "two" directory this command failed:

> require.resolve('lodash', { paths: '../one' })
Thrown:
Error: Cannot find module 'lodash'
Require stack:
- /Users/lloyd/Documents/tmp/demo/two/[eval]
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:616:15)
    at Function.resolve (internal/modules/cjs/helpers.js:21:19) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [ '/Users/lloyd/Documents/tmp/demo/two/[eval]' ]
}

I was confused because lodash definitely should be found in that path! But it was because I supplied an invalid paths type as input - it expects an array, not a string. This works:

> require.resolve('lodash', { paths: [ '../one' ] })
'/Users/lloyd/Documents/tmp/demo/one/node_modules/lodash/lodash.js'

So in this case, could we change the error message from "module not found" to "paths value must be an array"? That would help the developer more quickly understand and fix the issue.

@cjihrig
Copy link
Contributor

cjihrig commented May 8, 2019

@75lb I've opened #27613 to improve the error message.

cjihrig added a commit to cjihrig/node that referenced this issue May 9, 2019
This commit adds support for relative paths in
require.resolve()'s paths option.

PR-URL: nodejs#27598
Fixes: nodejs#27583
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
targos pushed a commit that referenced this issue May 10, 2019
This commit adds support for relative paths in
require.resolve()'s paths option.

PR-URL: #27598
Fixes: #27583
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
cjihrig added a commit to cjihrig/node that referenced this issue May 10, 2019
This commit adds input validation to require.resolve()'s
paths option. Prior to this change, passing in a non-array
value lead to a misleading 'module not found' error.

Refs: nodejs#27583
Trott pushed a commit to Trott/io.js that referenced this issue May 10, 2019
This commit adds input validation to require.resolve()'s
paths option. Prior to this change, passing in a non-array
value lead to a misleading 'module not found' error.

Refs: nodejs#27583

PR-URL: nodejs#27613
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
targos pushed a commit that referenced this issue May 11, 2019
This commit adds input validation to require.resolve()'s
paths option. Prior to this change, passing in a non-array
value lead to a misleading 'module not found' error.

Refs: #27583

PR-URL: #27613
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
RomainMuller added a commit to aws/aws-cdk that referenced this issue May 23, 2019
Node 12 has corrected how the require.resolve function applied un-documented behavior, so
that it is needed to make relative search paths explicit (./). The new approach should be
backwards-compatible.

Related: nodejs/node#27583
RomainMuller added a commit to aws/aws-cdk that referenced this issue May 23, 2019
Node 12 has corrected how the require.resolve function applied un-documented behavior, so
that it is needed to make relative search paths explicit (./).

Related: nodejs/node#27583
This was referenced Dec 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment