Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Draft
timmywil wants to merge1 commit intoimport-js:main
base:main
Choose a base branch
Loading
fromtimmywil:flat-config

Conversation

timmywil
Copy link

@timmywiltimmywil commentedJul 13, 2023
edited
Loading

  • utils/parse.js has support with tests for languageOptions, but languageOptions were never passed along in ExportMap

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 theimport/no-unused-modules rule. 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

regseb, eve0415, silverwind, JacobLey, and MikaelSiidorow reacted with thumbs up emoji
@timmywil
Copy link
Author

Tests seem to be trying to load a json file from iojs.org, which is currently down.
image

@ljharb
Copy link
Member

yep, you don't need to worry about that; i'll rerun it as needed.

timmywil reacted with thumbs up emoji

@timmywil
Copy link
Author

If anyone would like to point me in the right direction on how to fixno-unused-modules not finding an eslint config, I'd be happy to add that to this PR. I'm seeing this repo for the first time and still getting familiar.

@ljharbljharb marked this pull request as draftJuly 25, 2023 22:54
@ljharb
Copy link
Member

I've rebased this. If you can push up the tests you have, I can see if I can fix them.

@timmywil
Copy link
Author

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 toeslint.config.js and uncomment the import rule. Thesemi rule is just there to verify the config works without import.

ljharb reacted with eyes emoji

@timmywil
Copy link
Author

timmywil commentedJul 27, 2023
edited
Loading

I've dug into the tests a bit and noticed that the flat config test forno-unused-modules was always being skipped, but confirmed thatFlatRuleTester was defined locally. I don't know if it's related, but I went ahead and changed the way the test is skipped ifFlatRuleTester is not defined.

ljharb reacted with eyes emoji

@timmywiltimmywilforce-pushed theflat-config branch 4 times, most recently from32a625f toa2c841dCompareJuly 27, 2023 13:37
@ljharb
Copy link
Member

What's very strange with your repo is that theimport/no-unused-modules rule seems to work, unlessunusedExports is set.

@ljharb
Copy link
Member

ok, found the problem - whenunusedExports is true,src is required, and must match nonzero files.

@ljharb
Copy link
Member

Actually, that's not it either - it defaults toprocess.cwd() - so now I'm really confused :-) i can't reproduce the crash anymore either on top of some schema fixes i made. Let me land those, and rebase this, and we'll see what happens.

@ljharb
Copy link
Member

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
Copy link
Member

Looks like the issue is because theno-unused-modules rule is using FileEnumerator, which doesn't support the flat config (so it's throwing "configuration not found").

The actual fix for that may require some modifications to eslint itself.

@ljharb
Copy link
Member

cc@mdjermanovic who may have more context on eslint internals wrt flat config

@timmywil
Copy link
Author

timmywil commentedAug 14, 2023
edited
Loading

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 withoutFileEnumerator and@ljharb is understandably concerned about back-compat. However, I wonder if there's a middle ground. Perhaps we can initialize FileEnumerator in a way that reads flat config. WhileFileEnumerator uses legacy code, it uses a module that already has some flat config compat code available. It just doesn't use that.

@ljharb
Copy link
Member

@timmywil i'm all for a workaround if it works :-)

simlu reacted with eyes emoji

- utils/parse.js has support with tests for languageOptions,  but languageOptions were never passed along in ExportMap
@mdjermanovic
Copy link

Perhaps we can initialize FileEnumerator in a way that reads flat config. WhileFileEnumerator uses legacy code, it uses a module that already has some flat config compat code available. It just doesn't use that.

There's compat code that translates eslintrc configs into flat configs, but not the other way around. Either way,FileEnumerator is an internal module that will be removed in ESLint v10.0.0, so using it wouldn't be a permanent solution.

@ljharb
Copy link
Member

@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
Copy link
Author

There's compat code that translates eslintrc configs into flat configs, but not the other way around.

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?

ljharb reacted with thumbs up emoji

@keijokapp
Copy link

keijokapp commentedNov 8, 2023
edited
Loading

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.import/no-unused-modules could be handled as an independent issue.

@ljharb
Copy link
Member

@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.

keijokapp reacted with thumbs up emoji

@techmunk
Copy link

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 usecontext.filePaths, orcontext.eslintOptions to make better decisions on what to do. There's probably issues around rules mutating those objects in the current form, but to me that's less of a concern.context.filePaths is an array that looks like this:

[  {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
Copy link

@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)

@ljharb can you please open an issue in the eslint/eslint repo describing what data this rule needs from eslint?

@JounQin

This comment was marked as off-topic.

@timmywil
Copy link
Author

@JounQin could you explain how?languageOptions is still being dropped inchildContext() in that PR.

@JounQin

This comment was marked as off-topic.

@ljharb
Copy link
Member

@mdjermanoviceslint/eslint#18087

mdjermanovic reacted with thumbs up emoji

@clemyan
Copy link

Looks like there is an additional change needed so theecmaVersion andsourceType are read fromlangaugeOptions if they are not given inlanguageOptions.parserOptions,to mimic ESLint's behavior

ljharb reacted with eyes emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@ljharbljharbljharb left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

7 participants
@timmywil@ljharb@mdjermanovic@keijokapp@techmunk@JounQin@clemyan

[8]ページ先頭

©2009-2025 Movatter.jp