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(rule-finder): pass a file to getConfigForFile #306

Closed
wants to merge 1 commit into from

Conversation

kentcdodds
Copy link
Collaborator

This fixes a compatibility issue with eslint@6 which now passes the given file to path.resolve (https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/cli-engine/cli-engine.js#L905) which will throw if no file is given.

I tried this locally and it works without issue.

This fixes a compatibility issue with eslint@6 which now passes the given file to `path.resolve` (https://github.com/eslint/eslint/blob/1fb362093a65b99456a11029967d9ee0c31fd697/lib/cli-engine/cli-engine.js#L905) which will throw if no file is given.
@@ -23,7 +23,7 @@ function _getConfig(configFile) {
// Point to the particular config
configFile
});
return cliEngine.getConfigForFile();
Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't this only return the proper config for the config file, which might be yaml or .eslintrc json?

Config files are often not JS.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps. Consider this more of a bug report than a PR then. I'm not sure what the proper solution for this is. Because for this project there's not really a specific file that we're looking for a config for 🤔

Copy link
Collaborator

Choose a reason for hiding this comment

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

#304 is the bug report, which includes my latest attempt: master...ljharb:eslint_6

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🤦‍♂ I apologize. Totally didn't even think to check whether it had been reported. This is embarrassing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

No worries, to be fair I was super excited when I saw the PR notification because this package's v6 support is blocking me in two shared configs :-/

Choose a reason for hiding this comment

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

@ljharb (or anyone else looking for issues related to .js configs as I was and finding this comment)
re:

Config files often not JS.

by the way:
If you look at the top eslint configs on the first page of NPM, 12 out of the 18 use the .js config style (as opposed to .json or .yaml). The list of popular configs using this style includes:

(I already had this list)

Copy link
Collaborator

Choose a reason for hiding this comment

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

sure, but these are the top shared configs. I have 200 packages that only use the rc form, and a shared config for many of them that also does. Config files are indeed often not JS, even though they are also often JS :-)

@kentcdodds kentcdodds closed this Jul 10, 2019
@kentcdodds kentcdodds deleted the pr/eslint6compat branch July 10, 2019 17:45
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