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

Bug: ESLint class should not load plugins already defined in plugins #15079

Closed
1 task
fisker opened this issue Sep 18, 2021 · 8 comments · Fixed by #15187
Closed
1 task

Bug: ESLint class should not load plugins already defined in plugins #15079

fisker opened this issue Sep 18, 2021 · 8 comments · Fixed by #15187
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly repro:yes
Projects

Comments

@fisker
Copy link
Contributor

fisker commented Sep 18, 2021

Environment

Node version: v16.9.1
npm version: v7.9.0
Local ESLint version: v7.32.0
Global ESLint version: Not found
Operating System: win32 10.0.19043

What parser are you using?

Default (Espree)

What did you do?

import {ESLint} from 'eslint'

new ESLint({
	baseConfig: {
		extends: [
			'plugin:foo/preset',
		],
	},
	plugins: {
		foo: {
			configs: {
				preset: {}
			}
		}
	}
})

What did you expect to happen?

No errors.

What actually happened?

The config-array-factory is trying to load the plugin that I've set in the plugins.

node:internal/modules/cjs/loader:933
  const err = new Error(message);
              ^

Error: Failed to load plugin 'foo' declared in 'BaseConfig': Cannot find module 'eslint-plugin-foo'
Require stack:
- CWD\__placeholder__.js
Referenced from: BaseConfig
	// ...

Participation

  • I am willing to submit a pull request for this issue.

Additional comments

N/A

@fisker fisker added bug ESLint is working incorrectly repro:needed labels Sep 18, 2021
@eslint-github-bot eslint-github-bot bot added this to Needs Triage in Triage Sep 18, 2021
@nzakas
Copy link
Member

nzakas commented Sep 21, 2021

Strange this is only popping up now. Did this used to happen with CLIEngine?

@nzakas nzakas moved this from Needs Triage to Triaging in Triage Sep 21, 2021
@fisker
Copy link
Contributor Author

fisker commented Sep 22, 2021

I don't know, never used plugins before.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 23, 2021

CLIEngine didn't have a constructor option that accepts plugin implementations, but an instance method CLIEngine#addPlugin. As building a config array for baseConfig happens right away during new CLIEngine(), I believe this functionality didn't exist - it wasn't possible to define plugin:foo/preset in baseConfig and provide the foo plugin as an object instead of loading it from the file system.

@fisker
Copy link
Contributor Author

fisker commented Sep 23, 2021

If we add plugins to the CLIEngine constructor, we can init additionalPluginPool with the plugin implementations, will it solve the problem?

IDK how CascadingConfigArrayFactory works, just an idea.

@mdjermanovic
Copy link
Member

mdjermanovic commented Sep 23, 2021

If we add plugins to the CLIEngine constructor, we can init additionalPluginPool with the plugin implementations, will it solve the problem?

I think something like that would work. In the current version, we are updating additionalPluginPool on the instance, but that's too late for extends in baseConfig.

eslint/lib/eslint/eslint.js

Lines 435 to 454 in e3cd141

const processedOptions = processOptions(options);
const cliEngine = new CLIEngine(processedOptions);
const {
additionalPluginPool,
configArrayFactory,
lastConfigArrays
} = getCLIEngineInternalSlots(cliEngine);
let updated = false;
/*
* Address `plugins` to add plugin implementations.
* Operate the `additionalPluginPool` internal slot directly to avoid
* using `addPlugin(id, plugin)` method that resets cache everytime.
*/
if (options.plugins) {
for (const [id, plugin] of Object.entries(options.plugins)) {
additionalPluginPool.set(id, plugin);
updated = true;
}
}

We could instead pass additionalPluginPool map to the constructor. CLIEngine is now an internal module, so new options on it wouldn't be public.

@nzakas
Copy link
Member

nzakas commented Sep 24, 2021

We shouldn’t be making any further changes to CLIEngine at this point, as it will be going away. We need to start transitioning functionality into the ESLint class directly.

@nzakas
Copy link
Member

nzakas commented Oct 1, 2021

I took a look at this and updating CLIEngine is probably the easiest way to fix this.

@nzakas nzakas added the accepted There is consensus among the team that this change meets the criteria for inclusion label Oct 1, 2021
@nzakas nzakas moved this from Triaging to Ready to Implement in Triage Oct 1, 2021
@mdjermanovic
Copy link
Member

mdjermanovic commented Oct 18, 2021

I'm working on this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accepted There is consensus among the team that this change meets the criteria for inclusion archived due to age This issue has been archived; please open a new issue for any further discussion bug ESLint is working incorrectly repro:yes
Projects
Triage
Complete
Development

Successfully merging a pull request may close this issue.

3 participants