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() on Node 10 unexpectedly returns local JSON file #35367

Closed
Krinkle opened this issue Sep 27, 2020 · 4 comments
Closed

require.resolve() on Node 10 unexpectedly returns local JSON file #35367

Krinkle opened this issue Sep 27, 2020 · 4 comments
Labels
module Issues and PRs related to the module subsystem.

Comments

@Krinkle
Copy link
Contributor

Krinkle commented Sep 27, 2020

  • Version: v10.15.2
  • Platform: Linux
  • Subsystem: modules

What steps will reproduce the bug?

mkdir /tmp/bug && cd /tmp/bug
echo '{"boo":1}' > qunit.json
npm install --save-dev qunit
node
> require.resolve( "qunit", { paths: [ process.cwd() ] } )

What is the expected behavior?

To return /tmp/bug/node_modules/qunit/qunit/qunit.js.

What do you see instead?

It returns /tmp/bug/qunit.json.

Additional information

This works as expected on Node.js v14.5.0, but it is broken on Node v10 LTS. As such, the QUnit CLI is broken for users that have a qunit.json file in their repository. The CLI is unable to startup due to local module resolution leading QUnit to import the JSON file, instead of the qunit npm module.

The code in question comes from qunit:/src/cli/require-qunit.js, which is meant to return what require('qunit') would return if run from the user's project.

As such, results that point to local files that would normally need to be prefixed ./ are not expected to be returned, similar to how require('qunit') in that directory, could not return the result of require('./qunit').

In the case of wikimedia/eslint-config this is not easily worked around, because the qunit.json in question is part of this module's public interface, exposed to npm users and to ESLint via eslintrc files, e.g. "extends": "eslint-config-wikimedia/qunit".

@Krinkle
Copy link
Contributor Author

Krinkle commented Sep 27, 2020

I looked through the change logs, but was not able to find which commit fixed this. I did find #18408, which might be related?

I'm hoping such bug fix could perhaps be backported still to Node.js 10. But, I can also see how one might consider that as semver-major. I'd understand declining this.

If declining, I do hope someone could let me know if there is a way this could be worked around by QUnit in the interim (until Node 10 reaches end-of-life).

@aduh95
Copy link
Contributor

aduh95 commented Sep 27, 2020

Out of curiosity, what is the behaviour for require.resolve("qunit/")? My guess is that would not catch qunit.json but rather the node_modules/qunit folder – unless there is a ./qunit/index.js file or something like this maybe.

@ChALkeR ChALkeR added v10.x module Issues and PRs related to the module subsystem. labels Sep 27, 2020
@Krinkle
Copy link
Contributor Author

Krinkle commented Sep 30, 2020

@aduh95 That's right:

> process.versions.node
10.15.2
> require.resolve( "qunit", { paths: [ process.cwd() ] } )
'/eslint-config-wikimedia/qunit.json'
> require.resolve( "qunit/", { paths: [ process.cwd() ] } )
'/eslint-config-wikimedia/node_modules/qunit/qunit/qunit.js'

I also found another way, which makes it look at node_modules first:

> require.resolve( "qunit", { paths: [ process.cwd() + '/node_modules', process.cwd() ] } )
'/eslint-config-wikimedia/node_modules/qunit/qunit/qunit.js'

To my surprise, this works even when I misspell node_modules:

> require.resolve( "qunit", { paths: [ process.cwd() + '/zzz_not_here/' ] } )
'/eslint-config-wikimedia/node_modules/qunit/qunit/qunit.js'

Anyway, I think I'll go with adding node_modules for now, which seems least likely to change over time and/or regress other things I haven't thought of.

Krinkle added a commit to qunitjs/qunit that referenced this issue Sep 30, 2020
If the project using QUnit has a local qunit.json file in the
repository root, then the CLI was unable to find the 'qunit'
package. Instead, it first found a local 'qunit.json' file.

This affected an ESLint plugin with a 'qunit' preset file.

This bug was fixed in Node 12, and thus only affects Node 10 for us.

The bug is also specific to require.resolve(). Regular use of
require() was not affected and always correctly found the
local package. (A local file would only be considered if the
require started with dot-slash like `./qunit`.)

Ref nodejs/node#35367.
Fixes #1484.
Krinkle added a commit to qunitjs/qunit that referenced this issue Oct 3, 2020
If the project using QUnit has a local qunit.json file in the
repository root, then the CLI was unable to find the 'qunit'
package. Instead, it first found a local 'qunit.json' file.

This affected an ESLint plugin with a 'qunit' preset file.

This bug was fixed in Node 12, and thus only affects Node 10 for us.

The bug is also specific to require.resolve(). Regular use of
require() was not affected and always correctly found the
local package. (A local file would only be considered if the
require started with dot-slash like `./qunit`.)

Ref nodejs/node#35367.
Fixes #1484.
@targos
Copy link
Member

targos commented Apr 30, 2021

Closing as v10.x reached end of life.

@targos targos closed this as completed Apr 30, 2021
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

5 participants
@Krinkle @ChALkeR @targos @aduh95 and others