Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork1.5k
[Fix] pass languageOptions through in child context#2829
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
timmywil commentedJul 13, 2023
ljharb commentedJul 13, 2023
yep, you don't need to worry about that; i'll rerun it as needed. |
timmywil commentedJul 13, 2023
If anyone would like to point me in the right direction on how to fix |
ljharb commentedJul 25, 2023
I've rebased this. If you can push up the tests you have, I can see if I can fix them. |
timmywil commentedJul 27, 2023
I wasn't sure where to add the test, but I've created a repo to demonstrate the issue:https://github.com/timmywil/eslint-plugin-import-test Go to |
timmywil commentedJul 27, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I've dug into the tests a bit and noticed that the flat config test for |
Uh oh!
There was an error while loading.Please reload this page.
32a625f toa2c841dCompareljharb commentedJul 27, 2023
What's very strange with your repo is that the |
ljharb commentedJul 27, 2023
ok, found the problem - when |
ljharb commentedJul 27, 2023
Actually, that's not it either - it defaults to |
ljharb commentedJul 27, 2023
ok, well, tests are passing on the rebase, and the flat config tests are now running properly. I can also still reproduce the problem on your test repo, so I'm still looking into it. |
ljharb commentedJul 27, 2023
Looks like the issue is because the The actual fix for that may require some modifications to eslint itself. |
ljharb commentedJul 27, 2023
cc@mdjermanovic who may have more context on eslint internals wrt flat config |
timmywil commentedAug 14, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I was digging around in the eslint issues to see if this was discussed anywhere and it seems this is related totwoissues on eslint andone on eslint-plugin-import about any non-standard config name, which unfortunately includes flag config. It sounds like the preferred solution from eslint's POV would be to re-implement the rule without |
ljharb commentedAug 15, 2023
@timmywil i'm all for a workaround if it works :-) |
- utils/parse.js has support with tests for languageOptions, but languageOptions were never passed along in ExportMap
mdjermanovic commentedAug 24, 2023
There's compat code that translates eslintrc configs into flat configs, but not the other way around. Either way, |
ljharb commentedAug 24, 2023
@mdjermanovic what do you suggest for a permanent solution? (implied is one that will be guaranteed to match eslint's file enumeration rules as eslint evolves) |
timmywil commentedAug 25, 2023
yeah, I realized that later. I'm willing to contribute here, but I could use some direction on how to implement the rule without FileEnumerator. I imagine FileEnumerator is simply the legacy way of traversing files. How does flat config traverse files and could this rule use whatever it's using? |
keijokapp commentedNov 8, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Can this PR be merged as it is? It fixes most rules. For a while now I've kept a patched version of this library on my config package but this is sometimes causing package resolution issues. |
ljharb commentedNov 8, 2023
@keijokapp going from "we do not support flat config" to "we support it except for one rule which is horrible broken" isn't an improvement imo, since it will discourage people from using the rule in a way that will last long after it's eventually made to work. |
techmunk commentedNov 11, 2023
Without support from eslint, I don't think that rule can ever be made to work reliably. Flat config uses this method here to list fileshttps://github.com/eslint/eslint/blob/6d470d2e74535761bd56dcb1c021b463ef9e8a9c/lib/eslint/eslint-helpers.js#L471. It is not exposed or exported. Even if it was exposed, it would only work correctly if the rule context had access to the full flat config array object, the class for which is also something that I do not believe is exposed externally. To my mind, this requires ESLint to expose a way of rules getting access to the full config, and/or the list of files returned from the findFiles function mentioned above. I'm not sure on any reasoning as to why these cannot simply be put onto the context object given to the rule. This object is created athttps://github.com/eslint/eslint/blob/6d470d2e74535761bd56dcb1c021b463ef9e8a9c/lib/linter/linter.js#L989. The patch below, if applied to eslint would allow for a rule to use [ {filePath: '/full/path/to/project/file/javascript.js',ignored: false }, {filePath: '/full/path/to/project/file/code.js',ignored: false }]diff --git a/lib/eslint/flat-eslint.js b/lib/eslint/flat-eslint.jsindex 306c80de1..08ea6aa8a 100644--- a/lib/eslint/flat-eslint.js+++ b/lib/eslint/flat-eslint.js@@ -461,7 +461,9 @@ function verifyText({ fix, allowInlineConfig, reportUnusedDisableDirectives,- linter+ linter,+ filePaths,+ eslintOptions }) { const filePath = providedFilePath || "<text>";@@ -489,7 +491,9 @@ function verifyText({ */ filterCodeBlock(blockFilename) { return configs.isExplicitMatch(blockFilename);- }+ },+ filePaths,+ eslintOptions } );@@ -859,7 +863,9 @@ class FlatESLint { fix: fixer, allowInlineConfig, reportUnusedDisableDirectives,- linter+ linter,+ filePaths,+ eslintOptions }); /*diff --git a/lib/linter/linter.js b/lib/linter/linter.jsindex 9f29933ce..be988bda4 100644--- a/lib/linter/linter.js+++ b/lib/linter/linter.js@@ -965,7 +965,7 @@ const BASE_TRAVERSAL_CONTEXT = Object.freeze( * @param {string} physicalFilename The full path of the file on disk without any code block information * @returns {LintMessage[]} An array of reported problems */-function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageOptions, settings, filename, disableFixes, cwd, physicalFilename) {+function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageOptions, settings, filename, disableFixes, cwd, physicalFilename, filePaths, eslintOptionsn) { const emitter = createEmitter(); const nodeQueue = []; let currentNode = sourceCode.ast;@@ -1008,7 +1008,9 @@ function runRules(sourceCode, configuredRules, ruleMapper, parserName, languageO parserPath: parserName, languageOptions, parserServices: sourceCode.parserServices,- settings+ settings,+ filePaths,+ eslintOptions } ) );@@ -1377,7 +1379,9 @@ class Linter { options.filename, options.disableFixes, slots.cwd,- providedOptions.physicalFilename+ providedOptions.physicalFilename,+ providedOptions.filePaths,+ providedOptions.eslintOptions ); } catch (err) { err.message += `\nOccurred while linting ${options.filename}`;@@ -1752,7 +1756,9 @@ class Linter { options.filename, options.disableFixes, slots.cwd,- providedOptions.physicalFilename+ providedOptions.physicalFilename,+ providedOptions.filePaths,+ providedOptions.eslintOptions ); } catch (err) { err.message += `\nOccurred while linting ${options.filename}`; No idea if this solution would be acceptable to the eslint team. If someone wants to follow that up, that would be awesome. My job and family life get in the way of me being a suitable advocate for things like this. Links are based on latest eslint commit on main at time of this post. |
mdjermanovic commentedFeb 6, 2024
@ljharb can you please open an issue in the eslint/eslint repo describing what data this rule needs from eslint? |
This comment was marked as off-topic.
This comment was marked as off-topic.
timmywil commentedFeb 6, 2024
@JounQin could you explain how? |
This comment was marked as off-topic.
This comment was marked as off-topic.
ljharb commentedFeb 6, 2024
clemyan commentedJul 3, 2024
Looks like there is an additional change needed so the |

Uh oh!
There was an error while loading.Please reload this page.
I'm not sure if this needs more tests. The handling of languageOptions is tested in utils/parse.js.
This fixes many rules using flat config, but I am still hitting an issue with the
import/no-unused-modulesrule. I think that rule is trying to load the config again, but doesn't know how to load eslint.config.js.Related:
PR adding support for languageOptions to utils.parse:#2714
Issue to support flat config:#2556