feat(typescript-estree): replace globby w/ fast-glob
#9518
Conversation
|
Thanks for the PR, @SukkaW! typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community. The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately. Thanks again! 🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. |
✅ Deploy Preview for typescript-eslint ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9518 +/- ##
=======================================
Coverage 88.04% 88.04%
=======================================
Files 407 407
Lines 13948 13951 +3
Branches 4078 4079 +1
=======================================
+ Hits 12280 12283 +3
Misses 1355 1355
Partials 313 313
Flags with carried forward coverage won't be shown. Click here to find out more.
|
globby w/ fast-globglobby w/ fast-glob
globby w/ fast-globglobby w/ fast-glob
| - <tsconfigRootDir>/tsconfig.extra.json | ||
| - <tsconfigRootDir>/tsconfig.json |
There was a problem hiding this comment.
(passerby comment) Is there a small difference how these libraries find files, or why did this change? I don’t think it matters in this test as it’s just error message. :)
There was a problem hiding this comment.
Is there a small difference how these libraries find files
Yes, and thus results in an order difference.
There was a problem hiding this comment.
Hmm, ordering changes would make this either a bug, a bug fix, or a breaking change.
It looks to me like this is a bug. The project: ['./**/tsconfig.json', './**/tsconfig.extra.json'], indicates that tsconfig.json should come first. No?
There was a problem hiding this comment.
@JoshuaKGoldberg Fixed in the latest commits. Comments are added to explain the change.
cde05a7 to
ae480b4
Compare
| ).reduce<string[]>((acc, folder) => { | ||
| if (typeof folder === 'string') { | ||
| acc.push( | ||
| // prefix with a ! for not match glob | ||
| folder.startsWith('!') ? folder : `!${folder}`, | ||
| ); | ||
| } | ||
| return acc; | ||
| }, []); |
There was a problem hiding this comment.
A bonus change, replace the .reduce.map chain with a single .reduce to avoid unnecessary iteration.
4bd11fc to
15ac7be
Compare
|
@SukkaW would you consider updating this PR to use I took a naive stab at it in #9878, but it's failing the CI. I see you have a lot of extra logic here that I don't have, so perhaps that is needed. It seems you're much more familiar with how this project is setup, so if you're able to update this to use I was also curious why this is in draft mode as it appears to be working. Perhaps if you're able to update it to |
You can check my comments. |
globby w/ fast-globglobby w/ fast-glob
|
|
||
| if (globProjects.length > 0) { | ||
| // Although fast-glob supports multiple patterns, fast-glob returns arbitrary order of results | ||
| // to improve performance. To ensure the order is correct, we need to call fast-glob for each pattern |
There was a problem hiding this comment.
There is a unit test case against the order, so yes, it does matter.
There was a problem hiding this comment.
But why does the order matter? Shouldn't the unit test just check that the needed files are linted?
There was a problem hiding this comment.
Folks sometimes do things like ['packages/*/tsconfig.json', 'tsconfig.json'] - and/or entries like 'tsconfig.eslint.json'. The order intent is to first try the lower-level TSConfigs first.
PR Checklist
globbywithfast-glob#9453Overview
Addresses #9453. All tests passed on my machine locally.