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

Fix requireindex usage to use correct directory #144

Merged
merged 1 commit into from
Jan 18, 2021

Conversation

bmish
Copy link
Member

@bmish bmish commented Jan 14, 2021

There's an unreleased commit that switched the plugin to use requireindex for loading the rules: d3e0440. I noticed this broke the plugin for me.

eslint-plugin-qunit is included in a config in another linting plugin of mine. As a result, the requireindex call was checking the wrong current directory for lib/rules.

Oops! Something went wrong! :(

ESLint: 7.10.0

Error: Failed to load plugin 'qunit' declared in '.eslintrc.js » plugin:my-plugin/my-config': Cannot find module 'my-home-folder/eslint-plugin-qunit/lib'
Require stack:
- my-home-folder/eslint-plugin-qunit/node_modules/requireindex/index.js
- my-home-folder/eslint-plugin-qunit/index.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli-engine/config-array-factory.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli-engine/cascading-config-array-factory.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli-engine/cli-engine.js
- my-home-folder/my-repo/node_modules/eslint/lib/eslint/eslint.js
- my-home-folder/my-repo/node_modules/eslint/lib/eslint/index.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli.js
- my-home-folder/my-repo/node_modules/eslint/bin/eslint.js
Referenced from: my-home-folder/my-plugin/lib/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1029:15)
    at Function.Module._load (internal/modules/cjs/loader.js:898:27)
    at Module.require (internal/modules/cjs/loader.js:1089:19)
    at require (my-home-folder/my-repo/node_modules/v8-compile-cache/v8-compile-cache.js:161:20)
    at my-home-folder/eslint-plugin-qunit/node_modules/requireindex/index.js:11:28
    at Array.forEach (<anonymous>)
    at module.exports (my-home-folder/eslint-plugin-qunit/node_modules/requireindex/index.js:9:15)
    at Object.<anonymous> (my-home-folder/eslint-plugin-qunit/index.js:13:12)
    at Module._compile (my-home-folder/my-repo/node_modules/v8-compile-cache/v8-compile-cache.js:194:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1220:10)
error Command failed with exit code 2.

@bmish bmish changed the title Fix requireinde usage to use correct directory Fix requireindex usage to use correct directory Jan 14, 2021
…gins

eslint-plugin-qunit is included in a config in another linting plugin of mine. As a result, the requireindex call was checking the wrong current directory for lib/rules.

```
Oops! Something went wrong! :(

ESLint: 7.10.0

Error: Failed to load plugin 'qunit' declared in '.eslintrc.js » plugin:my-plugin/my-config': Cannot find module 'my-home-folder/eslint-plugin-qunit/lib'
Require stack:
- my-home-folder/eslint-plugin-qunit/node_modules/requireindex/index.js
- my-home-folder/eslint-plugin-qunit/index.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli-engine/config-array-factory.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli-engine/cascading-config-array-factory.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli-engine/cli-engine.js
- my-home-folder/my-repo/node_modules/eslint/lib/eslint/eslint.js
- my-home-folder/my-repo/node_modules/eslint/lib/eslint/index.js
- my-home-folder/my-repo/node_modules/eslint/lib/cli.js
- my-home-folder/my-repo/node_modules/eslint/bin/eslint.js
Referenced from: my-home-folder/my-plugin/lib/index.js
    at Function.Module._resolveFilename (internal/modules/cjs/loader.js:1029:15)
    at Function.Module._load (internal/modules/cjs/loader.js:898:27)
    at Module.require (internal/modules/cjs/loader.js:1089:19)
    at require (my-home-folder/my-repo/node_modules/v8-compile-cache/v8-compile-cache.js:161:20)
    at my-home-folder/eslint-plugin-qunit/node_modules/requireindex/index.js:11:28
    at Array.forEach (<anonymous>)
    at module.exports (my-home-folder/eslint-plugin-qunit/node_modules/requireindex/index.js:9:15)
    at Object.<anonymous> (my-home-folder/eslint-plugin-qunit/index.js:13:12)
    at Module._compile (my-home-folder/my-repo/node_modules/v8-compile-cache/v8-compile-cache.js:194:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1220:10)
error Command failed with exit code 2.
```
@bmish bmish force-pushed the fix-require-index branch from 3885410 to 70ca2e2 Compare January 14, 2021 23:38
@coveralls
Copy link

coveralls commented Jan 14, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling 70ca2e2 on bmish:fix-require-index into 7fb119d on platinumazure:master.

Copy link
Collaborator

@platinumazure platinumazure left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The change looks good generally.

Do you think it might be possible to add a test case? I would really like to catch this in CI if there's a regression.

@bmish
Copy link
Member Author

bmish commented Jan 15, 2021

A test would be great but I don't immediately know how to test this...

@platinumazure
Copy link
Collaborator

A test would be great but I don't immediately know how to test this...

@bmish It seemed like you could recreate it pretty easily outside of the package. I assume there's something about being inside the package that makes this difficult to test. Can you describe the issue you're seeing (or foreseeing) here? Maybe we can come up with something.

Off the cuff, the first thing I can think of is not being able to require("eslint-plugin-qunit") from inside the plugin (or rather, inside the test suite). If that's the case, could it be faked using a nested package.json inside a subdirectory of the tests directory?

@bmish
Copy link
Member Author

bmish commented Jan 18, 2021

I experimented with testing for a while and didn't have any luck. Tried a few things (using process.cwd() / process.chdir() before require('index.js'), eslint.calculateConfigForFile('index.js')) but was unable to repro from the issue from a test.

I did however check to confirm that other usages of requireindex on GitHub all appear to use __dirname: https://github.com/search?l=JavaScript&q=requireindex&type=Code

@platinumazure
Copy link
Collaborator

Thanks for taking the time @bmish, I appreciate it. Guess we can merge this as it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants