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] pass languageOptions through in child context #2829

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

timmywil
Copy link

@timmywil timmywil commented Jul 13, 2023

  • 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 the import/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

@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

ljharb commented Jul 13, 2023

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

@timmywil
Copy link
Author

If anyone would like to point me in the right direction on how to fix no-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.

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

ljharb commented Jul 25, 2023

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

@timmywil
Copy link
Author

timmywil commented Jul 27, 2023

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

@timmywil timmywil force-pushed the flat-config branch 4 times, most recently from 32a625f to a2c841d Compare July 27, 2023 13:37
@ljharb
Copy link
Member

ljharb commented Jul 27, 2023

What's very strange with your repo is that the import/no-unused-modules rule seems to work, unless unusedExports is set.

@ljharb
Copy link
Member

ljharb commented Jul 27, 2023

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

@ljharb
Copy link
Member

ljharb commented Jul 27, 2023

Actually, that's not it either - it defaults to process.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

ljharb commented Jul 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
Copy link
Member

ljharb commented Jul 27, 2023

Looks like the issue is because the no-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

ljharb commented Jul 27, 2023

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

@timmywil
Copy link
Author

timmywil commented Aug 14, 2023

I was digging around in the eslint issues to see if this was discussed anywhere and it seems this is related to two issues on eslint and one 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 FileEnumerator 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. While FileEnumerator 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

ljharb commented Aug 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
Copy link

Perhaps we can initialize FileEnumerator in a way that reads flat config. While FileEnumerator 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

ljharb commented Aug 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
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?

jgarber623 added a commit to jgarber623/eslint-config that referenced this pull request Oct 5, 2023
It's got some in-the-weeds issues with passing around data internally
that makes the plugin incompatible with flat config:

- import-js/eslint-plugin-import#2556
- import-js/eslint-plugin-import#2829
jgarber623 added a commit to jgarber623/eslint-config that referenced this pull request Oct 5, 2023
* Add eslint-plugin-array-func package

* Configure eslint-plugin-array-func

* Add more ESLint plugins

* Add glue-y flat config compat code for plugins

* Configure more ESLint plugins

* Configure sort-keys rule

* Fix object key sorting in test files

* Add and configure eslint-plugin-regexp package

* Remove @eslint/eslintrc

* Add @eslint/js package and update config, tests

* Add and configure eslint-plugin-unicorn

* Ditch JSDOC comments in this project

* Add and configure sort-class-members plugin

* Ignore coverage folder

* Remove JSDOC-style comments

* Remove (for now) eslint-plugin-import

It's got some in-the-weeds issues with passing around data internally
that makes the plugin incompatible with flat config:

- import-js/eslint-plugin-import#2556
- import-js/eslint-plugin-import#2829
@keijokapp
Copy link

keijokapp commented Nov 8, 2023

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

ljharb commented Nov 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
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 files https://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 at https://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 context.filePaths, or context.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.js
index 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.js
index 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

timmywil commented Feb 6, 2024

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

@JounQin

This comment was marked as off-topic.

@ljharb
Copy link
Member

ljharb commented Feb 6, 2024

@mdjermanovic eslint/eslint#18087

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants