search

import-js / eslint-plugin-import

ESLint plugin with rules that help validate proper imports.
MIT License
5.57k stars 1.57k forks source link

[utils] [fix] `parse`: espree parser isn't working with flat config #3061

Closed michaelfaith closed 1 month ago

michaelfaith commented 2 months ago

This change addresses an issue with using the espree parser with flat config. keysFromParser is responsible for finding the visitorKeys from a combination of the parser instance, parserPath, and the results of a call to parseForESLint. When using flat config, the parserPath will not be on the context object, and when using the espree parser there are no results from parseForESLint. The espree parser does provide visitor keys as a prop on the parser itself. However, this logic was only returning it when it found that "espree" was somewhere in the parserPath (which doesn't exist on flat config).

I changed it so that it first does the check for the old-school babel parser using parser path, and then just checks the parser instance for the presence of VisitorKeys, rather than using parserPath at all. The reason for the re-order is that if the old babel-eslint parser, for some reason has VisitorKeys, having the new condition first, would change the behavior.

michaelfaith commented 2 months ago

Thinking i should maybe revert the addition of mjs and cjs extensions to the default extensions list that I made in the flat config pr, since that was more or less an after thought / not central to the flat config support, and could be causing inadvertent regression? What do you think?

G-Rath commented 2 months ago

fwiw this doesn't seem to have changed or improved anything in my local samples or in #2996

michaelfaith commented 2 months ago

fwiw this doesn't seem to have changed or improved anything in my local samples or in #2996

Yeah, like I mentioned here https://github.com/import-js/eslint-plugin-import/pull/2996#issuecomment-2351643614 this will need more work for the v9 tests, due to how the v9 RuleTester is wrapping the parser object. Are you able to share your v8 repro? Maybe updating the flat example in the root of the repo with a reproduction? I can look closer at it, but I think this change is still needed regardless.

G-Rath commented 2 months ago

I've pushed up https://github.com/G-Rath/eslint-plugin-import-debugging - npm i && bash run.sh should do the trick

michaelfaith commented 2 months ago

I've pushed up https://github.com/G-Rath/eslint-plugin-import-debugging - npm i && bash run.sh should do the trick

I just added the equivalent test to the legacy and flat examples in this repo and they're both reporting error for no-cycle. I added those files to this PR (though, changed it to warn, so-as not to fail ci).

image

G-Rath commented 2 months ago

I don't follow - are you saying that with my repository you're getting no-cycle errors on both legacy and flat config?

michaelfaith commented 2 months ago

I don't follow - are you saying that with my repository you're getting no-cycle errors on both legacy and flat config?

I wanted to test with this branch, so I just recreated the test in the existing examples here, and yes, it's detecting no-cycle correctly with both.

G-Rath commented 2 months ago

it's detecting no-cycle correctly with both.

sorry for the doubt but I want to be extra clear since that doesn't match my results locally - you're saying with a fresh clone of https://github.com/G-Rath/eslint-plugin-import-debugging, running just npm i && bash run.sh without any modifications to files, you're getting no-cycle erroring four times?

Not saying it's not possible, but it's annoying (for me) if that is the case 😅 I assume I've got a local cache issue

michaelfaith commented 2 months ago

it's detecting no-cycle correctly with both.

sorry for the doubt but I want to be extra clear since that doesn't match my results locally - you're saying with a fresh clone of https://github.com/G-Rath/eslint-plugin-import-debugging, running just npm i && bash run.sh without any modifications to files, you're getting no-cycle erroring four times?

Not saying it's not possible, but it's annoying if that is the case 😅 I assume I've got a local cache issue

That wouldn't test it with this code change. Your repo is pulling the release version.
image

Check out the examples folders. It's a very simple setup, just like yours. But it's testing this code. And I have a windows computer, so can't run sh files anyway. But like I said, it's testing the same thing.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 66.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 94.92%. Comparing base (f72f207) to head (5c91996).

Files with missing lines Patch % Lines
utils/parse.js 66.66% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #3061 +/- ## =========================================== + Coverage 82.18% 94.92% +12.73% =========================================== Files 93 82 -11 Lines 4136 3563 -573 Branches 1391 1251 -140 =========================================== - Hits 3399 3382 -17 + Misses 737 181 -556 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

G-Rath commented 2 months ago

Sure but either way we need to establish a baseline - happy to have this conversation over on #2996 if you think it's confusing to have here, but either way my request is the same.

fwiw I have applied your fix here to the repo manually and it didn't change the output.

My whole point with that repo is it's a behaviour that does not seem to be being caught by the test suite, so making changes here is pretty moot because I would expect you to not have matching behaviour.

I've now run my repo on two different laptops which give the same result (only legacy config gives no-cycle errors), which'd indicate this isn't a caching issue

michaelfaith commented 2 months ago

Sure but either way we need to establish a baseline - happy to have this conversation over on #2996 if you think it's confusing to have here, but either way my request is the same.

fwiw I have applied your fix here to the repo manually and it didn't change the output.

My whole point with that repo is it's a behaviour that does not seem to be being caught by the test suite, so making changes here is pretty moot because I would expect you to not have matching behaviour.

I've now run my repo on two different laptops which give the same result (only legacy config gives no-cycle errors), which'd indicate this isn't a caching issue

It occured to me that the examples in this repo are both set up with the TS parser (oops)😅 So, i pulled your repo, and discovered the issue there. For one, I had to add both the changes from this PR and this PR: https://github.com/import-js/eslint-plugin-import/pull/3052 since it's not released yet. After doing that, it still wasn't reporting the error. But after debugging I found that the call to espree's parse function was returning this error "sourceType 'module' is not supported when ecmaVersion < 2015. Consider adding{ ecmaVersion: 2015 }to the parser options.". And sure enough, I copied ecmaVersion into languageOptions.parserOptions instead of just languageOptions and it parses ok, and reports no-cycle. It seems like that's something the espree parser needs directly as well.

image

G-Rath commented 2 months ago

hmm ok that's interesting - something still seems off there to me.

Pulling together a few things:

michaelfaith commented 2 months ago

Ah, in that case you're right. I didn't see this blurb at the bottom of the page haha.

image

We aren't passing ecmaVersion to the parse function. We're just passing what's in parserOptions (which is why adding it to parserOptions in the config made it work). That's an easy fix.

It's interesting that they moved sourceType and ecmaVersion out of parserOptions but still pass them in as parser options. 🤔

michaelfaith commented 2 months ago

Updated with that change. And verified that it works in your debugging repo without needing to change the config.
image

Thanks for helping to identify that gap.

michaelfaith commented 2 months ago

Added a unit test to cover the ecmaVersion and sourceType fix