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/add-error-message-for-other-module-not-found #8

Conversation

JohnDaly
Copy link
Contributor

@JohnDaly JohnDaly commented Jun 9, 2022

If there is a require statement in the 'eslint-local-rules' file, which results in a MODULE_NOT_FOUND error, its not immediately obvious with the current error messaging.

// eslint-local-rules.js

// Will cause a 'MODULE_NOT_FOUND' error
require('bad-import-which-will-not-resolve');

module.exports = {
  'disallow-identifiers': {
    meta: {
      docs: {
        description: 'disallow identifiers',
        category: 'Possible Errors',
        recommended: false,
      },
      schema: [],
    },
    create: function(context) {
      return {
        Identifier: function(node) {
          context.report({
            node: node,
            message: 'Identifiers not allowed for Super Important reasons.',
          });
        },
      };
    },
  },
};

Will result in the following error:

 Error: eslint-plugin-local-rules: Cannot find "eslint-local-rules{.js,.cjs} or eslint-local-rules/index.js (checked all
    ancestors of "path/to/eslint-plugin-local-rules").

This PR will make it so that the Cannot find module 'bad-import-which-will-not-resolve' error will be thrown

@cletusw
Copy link
Owner

cletusw commented Jun 10, 2022

Great find & fix! I simplified the code a little -- let me know if it's still good and I'll get it merged, thanks!

@JohnDaly
Copy link
Contributor Author

Great find & fix! I simplified the code a little -- let me know if it's still good and I'll get it merged, thanks!

Looks great!

@cletusw cletusw merged commit e95ca30 into cletusw:master Jun 10, 2022
@JohnDaly JohnDaly deleted the fix/add-error-message-for-other-module-not-found branch June 10, 2022 05:55
@cletusw
Copy link
Owner

cletusw commented Jun 10, 2022

Merged and released v1.1.1, thanks for the fix!

@diegocr
Copy link

diegocr commented Jun 10, 2022

Guys, I think this PR is causing the issue pointed in #7 since i never encountered it before with 1.1.0

Error: Failed to load plugin 'local-rules' declared in '.eslintrc.json': eslint-plugin-local-rules: Cannot find "eslint-local-rules{.js,.cjs} or eslint-local-rules/index.js (...).

Noteworthy, i don't have any require() call into my eslint-local-rules.js

Can you please look at fixing this at your earliest convenience? Thanks in advance!

cletusw added a commit that referenced this pull request Jun 10, 2022
cletusw added a commit that referenced this pull request Jun 10, 2022
@cletusw
Copy link
Owner

cletusw commented Jun 10, 2022

@diegocr Sorry about that!! I was literally just thinking how bad it is this thing doesn't have a single test. Reverted this changed and published as v1.1.2. I'll try again more carefully -- if you have a simplified version of your setup that errors in v1.1.1 that I can test against that would be great, thanks.

@@ -24,20 +24,20 @@ module.exports = {
// Similar to native `require` behavior, but doesn't check in `node_modules` folders
// Based on https://github.com/js-cli/node-findup-sync
function requireUp(filename, cwd) {
var filepath = path.resolve(cwd, filename);
var filepath = path.resolve(cwd, filename) + exts[i];
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cletusw I missed this earlier, but filepath needs to be defined within the scope of the for loop so that it can access i

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In v1.1.1, the filepath variable evaluates to /path/to/eslint-local-rulesundefined.

This will cause MODULE_NOT_FOUND errors, an example might look like:

Error: Cannot find module '/path/to/eslint-local-rulesundefined'

Since this error message matches the pattern Cannot find module '${filepath}' that we check for, an error is not thrown, and this results in the standard 'Failed to load plugin' error being logged

@JohnDaly JohnDaly mentioned this pull request Jun 11, 2022
cletusw pushed a commit that referenced this pull request Jun 12, 2022
See #8 

Also:
* Set up tests with Vitest
* Add prettier
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

Successfully merging this pull request may close these issues.

3 participants