Skip to content

Conversation

Josh194
Copy link
Contributor

@Josh194 Josh194 commented May 15, 2025

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

Previously, attempting to use an overridden config containing a plugin with a circular structure (as is suggested for flat configs) would error, as the caching implementation would attempt to stringify said plugins in order to use them as cache keys.

This PR fixes the issue by replacing the cache object with a Map, and using the plugin objects directly as keys, for whom equality can be checked even in the presence of circular references.

A test case (verified to error without the fix) is also included.

Fixes: #270.

Breaking Changes

None (unless jsonStringifyReplacerSortKeys was somehow externally visible in a way I didn't notice).

Additional Info

Test case plugin code is taken almost exactly from https://eslint.org/docs/latest/extend/plugin-migration-flat-config#migrating-configs-for-flat-config; it is probably possible to make it a bit shorter, but I didn't bother to look into it.

npm run test was run immediately prior to the final (and only) commit, so any stylistic lints should have been applied, but apologies if I've missed anything.

Replace the getESLint.js cache object with a Map, using object keys instead of JSON strings.
Copy link

linux-foundation-easycla bot commented May 15, 2025

CLA Signed


The committers listed above are authorized under a signed CLA.

@Josh194
Copy link
Contributor Author

Josh194 commented May 15, 2025

Just realized there's an issue with the object map solution; any circular plugins will never be cached, since the Map only looks for referential equality. This alone is not really a regression per-se (any options that were working before could not have been circular, so this issue doesn't affect them), but it does mean that loadESLintThreaded will never see the same cache keys for circular plugins as getESLint in the cleanup function (getESLint.js:86). It's a little unclear to me if this is actually a problem at the moment (JS is not a language I have much time with); I'll look closer in a little bit.

Ideally this would be fixed by using a map that supports deep comparisons, but a much easier solution would just be to improve the JSON conversion as suggested in #270. The loadESLintThreaded issue (assuming it is an issue) could potentially be solved by having it delegate to a private function that takes a cache key directly, which getESLint could then pass in (this is assuming that the threaded cache key is actually supposed to match the outer one, which I haven't checked yet), but doing so would only be a partial fix anyway, so probably not the best solution.

@Josh194
Copy link
Contributor Author

Josh194 commented May 15, 2025

Actually, this breaks caching entirely; I forgot object equality in JS is based completely on reference equality. Going to close this since I think losing caching far outweighs the fix itself, but I'll make another PR in a moment with the stringify approach if I can get that to work.

@Josh194 Josh194 closed this May 15, 2025
/** @type {{[key: string]: any}} */
const cache = {};
/** @type {Map<CacheKey, any>} */
const cache = new Map();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to use WeakMap? Otherwise it is memory leak for incremental builds

Copy link

codecov bot commented May 15, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.65%. Comparing base (3adbe4c) to head (dbe64a5).
Report is 23 commits behind head on master.

Additional details and impacted files
@@             Coverage Diff             @@
##            master     #276      +/-   ##
===========================================
- Coverage   100.00%   98.65%   -1.35%     
===========================================
  Files            7        7              
  Lines          291      297       +6     
  Branches        81       83       +2     
===========================================
+ Hits           291      293       +2     
- Misses           0        4       +4     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Josh194 Josh194 deleted the pull branch May 17, 2025 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

"Maximum call stack size exceeded" when running with eslnt 9

2 participants