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

[michaelfaith]

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]

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]

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

[michaelfaith]

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

Yeah, like I mentioned here #2996 (comment) 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]

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

[michaelfaith]

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

[G-Rath]

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

[michaelfaith]

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]

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]

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.

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]

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 ⚠️

Additional details and impacted files

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[G-Rath]

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]

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: #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.

[G-Rath]

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

Pulling together a few things:

• I had forgotten that [Fix] ExportMap / flat config: include languageOptions in context #3052 hadn't been released, and either way that's an important bug fix we shouldn't be forgetting about
• I had discovered a similar sounding bug which I fixed in my v9 PR: b80bd59
• according to the docs ecmaVersion should always be passed to parsers
  • [New] support eslint 9 #2996 (comment) & https://eslint.org/docs/latest/use/configure/parser
• so I think there's a bug somewhere be it this plugin, eslint, or espree...

[michaelfaith]

• according to the docs ecmaVersion should always be passed to parsers

  • feat: support ESLint v9 #2996 (comment) & https://eslint.org/docs/latest/use/configure/parser

• so I think there's a bug somewhere be it this plugin, eslint, or espree...

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

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]

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

Thanks for helping to identify that gap.

[michaelfaith]

Added a unit test to cover the ecmaVersion and sourceType fix