[Fix] `exportMap`: export map cache is tainted by unreliable parse results

[michaelfaith]

This change addresses an issue observed with the v9 upgrade, where the ExportMap Cache is being tainted with a bad export map, if the parse doesn't yield a visitorKeys (which can happen if an incompatible parser is used (e.g. og babel eslint)). If one run of the no-cycle rule uses an incompatible parser and taints the cache and a subsequent test is run with a compatible parser, then the bad export map will be found in the cache and used instead of attempting to proceed with the parse.

I don't think this is likely to occur in the wild, but in the event that it does, this should help. One thing to note, though: if someone is using an incompatible parser, then this change will have a performance impact, since it will attempt to create an export map every time, until it yields one that has visitorKeys attached.

I also updated the getExports test to use a valid parserPath, rather than a mocked one, so the tests are acting on a valid parserPath. Otherwise the export map won't be cached following this change and the test for it caching would fail.

Additional context: https://github.com/import-js/eslint-plugin-import/pull/2996#issuecomment-2351204273

[michaelfaith]

The test failures are from eslint 2 and 3, which I'm guessing doesn't yield a visitorKeys from the parse, causing the cache to be bypassed (failing the test that the export map is cached).

[codecov[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.72%. Comparing base (5c9757c) to head (8117601).

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #3062 +/- ## ========================================== - Coverage 92.21% 90.72% -1.50% ========================================== Files 82 82 Lines 3557 3557 Branches 1244 1244 ========================================== - Hits 3280 3227 -53 - Misses 277 330 +53 ``` | [Flag](https://app.codecov.io/gh/import-js/eslint-plugin-import/pull/3062/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=import-js) | Coverage Δ | | |---|---|---| | [](https://app.codecov.io/gh/import-js/eslint-plugin-import/pull/3062/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=import-js) | `90.72% <100.00%> (-1.50%)` | :arrow_down: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=import-js#carryforward-flags-in-the-pull-request-comment) to find out more.

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

[michaelfaith]

Another idea for how to approach this, to limit the impact to just the rules that need it:
we could add an additional optional param to ExportMapBuilder.get that takes a skip cache function, which takes an export map and returns a boolean for whether that instance of the map should be cached. no-cycle could then pass in (exportMap) => !exportMap.visitorKeys, which has the same effect, but limits it to only that rule.

[G-Rath]

Confirming that this looks to address some of the test failures in #2996 - definitely the ones from no-cycle and probably others.

The test failures are from eslint 2 and 3

Could you just also check what version of ESLint is being used? as this seems to only take effect when using flat config and/or ESLint v9 which neither of those versions will support, and it feels like it could be more of a footgun to have rules control the cache

[michaelfaith]

Could you just also check what version of ESLint is being used? as this seems to only take effect when using flat config and/or ESLint v9 which neither of those versions will support, and it feels like it could be more of a footgun to have rules control the cache

Yeah, I like that too.

[michaelfaith]

This approach seems fine to me. If a version check is the cleanest way to handle this, that's fine, but it'd be much nicer to feature-test instead.

Updated the tests to only run the caching test if we determine babel-eslint has visitor-keys.

cc @G-Rath you may want to pull this into your PR, to resolve the v2/v3 test errors.