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: load plugins in the CLI in flat config mode #18185
Conversation
✅ Deploy Preview for docs-eslint canceled.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Just a few small questions/suggestions, nothing blocking IMO.
+1 on the tests, nice and thorough!
lib/cli.js
Outdated
|
||
await Promise.all(pluginNames.map(async pluginName => { | ||
|
||
const shortName = naming.getShorthandName(pluginName, "eslint-plugin"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Style] Small thing: shortName
isn't used until after the if
. I think it'd make the code just a bit more clear to move the variable down a few lines. (Is this too nitpicky a comment?)
lib/cli.js
Outdated
const module = await importer.import(longName); | ||
|
||
if (!("default" in module)) { | ||
throw Error(`"${longName}" cannot be imported because it does not provide a default export`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Question] Elsewhere outside of .md
files and tests/
, it's always throw new Error
- not throw Error
. I.e. no-throw-literal
has been adhered to. Is it intentional that this doesn't?
throw Error(`"${longName}" cannot be imported because it does not provide a default export`); | |
throw new Error(`"${longName}" cannot be imported because it does not provide a default export`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's just a matter of style. throw Error()
is the same as throw new Error()
. no-throw-literal
does not flag it (playground). In my repos, I have a rule that reports unnecessary new
expressions, so I have a tendency to write just Error()
. eslint-plugin-unicorn
has this rule that requires new Error
in throw statements.
/** | ||
* Loads plugins with the specified names. | ||
* @param {{ "import": (name: string) => Promise<any> }} importer An object with an `import` method called once for each plugin. | ||
* @param {string[]} pluginNames The names of the plugins to be loaded, with or without the "eslint-plugin-" prefix. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's fine to allow both, but we can start recommending using the full package name. In the long run, eslintrc will go away and plugin names are not enforced to be eslint-plugin-xxx
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aladdin-add indeed the plugins used with this option must still follow eslintrc naming convention, but since this PR doesn't change that behavior, can you open a separate issue to discuss this?
I think there's no need to add more tests for eslintrc since there are already some tests and the way the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! 👍
tests/lib/cli.js
Outdated
@@ -1649,6 +1649,105 @@ describe("cli", () => { | |||
}); | |||
}); | |||
|
|||
describe("flat Only", () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was probably accidentally inserted in "eslintrc Only".
cli
eslintrc Only
flat Only
`--plugin` option
√ should load a plugin from a CommonJS package (41ms)
√ should load a plugin from an ESM package
√ should load multiple plugins
√ should resolve plugins specified with 'eslint-plugin-'
√ should resolve plugins in the parent directory's node_module subdirectory
√ should fail if a plugin is not found
√ should fail if a plugin throws an error while loading
√ should fail to load a plugin from a package without a default export
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed that, thanks! It's fixed in 34e02cb.
lib/cli.js
Outdated
const module = await importer.import(longName); | ||
|
||
if (!("default" in module)) { | ||
throw new Error(`"${longName}" cannot be imported because it does not provide a default export`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like the package doesn't provide a default export, but it does while the problem is that its default export doesn't have "default" name exported. Maybe something like this?
throw new Error(`"${longName}" cannot be imported because it does not provide a default export`); | |
throw new Error(`"${longName}" cannot be used with the `--plugin` option because its default module does not provide `default` export`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in e6a5f4e.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[X] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[ ] Add something to the core
[ ] Other, please explain:
Fixes #18159
What changes did you make? (Give an overview)
The changes in this PR relate to how the CLI option
--plugin
works in flat config mode:default
property of the imported module is now used. This aligns the plugin loading behavior with eslintrc and with the recommendations for creating a custom plugin.This PR also adds unit tests and a new note to the custom plugins docs.
Is there anything you'd like reviewers to focus on?
Some of the new unit tests in this PR, the ones with CommonJS plugins, can be generalized to cover the eslintrc case. Do we want to add tests for eslintrc to make sure that the behavior in flat config mode is similar?