Skip to content

fix(typescript-estree): replace fast-glob with tinyglobby #10544

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 7 commits into from

Conversation

james-pre
Copy link

@james-pre james-pre commented Dec 23, 2024

PR Checklist

Overview

This PR migrates from fast-glob to tinyglobby. For more information, you can see the issue that this PR fixes: #10533

@typescript-eslint
Copy link
Contributor

Thanks for the PR, @james-pre!

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.

Copy link

netlify bot commented Dec 23, 2024

Deploy Preview for typescript-eslint ready!

Name Link
🔨 Latest commit 42eb0b2
🔍 Latest deploy log https://app.netlify.com/sites/typescript-eslint/deploys/6769e15bc4ae310008db55ee
😎 Deploy Preview https://deploy-preview-10544--typescript-eslint.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
1 paths audited
Performance: 76 (🟢 up 3 from production)
Accessibility: 100 (no change from production)
Best Practices: 92 (no change from production)
SEO: 98 (no change from production)
PWA: 80 (no change from production)
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

nx-cloud bot commented Dec 23, 2024

View your CI Pipeline Execution ↗ for commit 42eb0b2.

Command Status Duration Result
nx run-many --target=build --exclude website --... ✅ Succeeded 5s View ↗
nx run-many --target=clean ✅ Succeeded 10s View ↗

☁️ Nx Cloud last updated this comment at 2024-12-23 22:34:34 UTC

@james-pre james-pre changed the title fix(typescript-estree): Replace fast-glob with tinyglobby fix(typescript-estree): replace fast-glob with tinyglobby Dec 23, 2024
@james-pre
Copy link
Author

james-pre commented Dec 23, 2024

The error message for the test doesn't make sense to me, as _tinyglobby doesn't even exist in the scope:


 FAIL  tests/lib/parse.test.ts
  ● Test suite failed to run
    ReferenceError: Cannot access '_tinyglobby' before initialization
      45 |   return {
      46 |     ...tg,
    > 47 |     globSync: jest.fn(tinyglobby.globSync),
         |                       ^
      48 |   };
      49 | });
      50 |
      at tinyglobby (tests/lib/parse.test.ts:47:23)
      at Object.<anonymous> (tests/lib/parse.test.ts:34:61)

Some insight would be appreciated.

@JoshuaKGoldberg
Copy link
Member

@james-pre I appreciate the enthusiasm for getting this in 🙂. But was there a reason you deleted the PR template? We have it because we like it and it makes sure folks know the steps they're meant to take before sending a PR.

I'm converting this to draft because the issue hasn't been accepted and there are some test failures. Feel free to keep working on it in the meantime, it's great to see it almost fully working! 🚀

@JoshuaKGoldberg JoshuaKGoldberg marked this pull request as draft December 23, 2024 21:32
Co-authored-by: Josh Goldberg ✨ <git@joshuakgoldberg.com>
@james-pre
Copy link
Author

Sorry on the PR check list, I re-added it. Please let me know if there is anything I can do to move this forward.

@JoshuaKGoldberg
Copy link
Member

Thanks! There are two blockers I think:

For issue input, we're just waiting. Given that this is holiday season it might be a business week or two till we get that.

For CI, that's something you could do.

James Prevett and others added 4 commits December 23, 2024 16:04
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
@JoshuaKGoldberg
Copy link
Member

Per #10533 (comment) we're going to hold off for a few months on the switch. tinyglobby is not stable enough yet for us to use it.

This is a big part of why we ask that folks go by the contributing guidelines. It feels bad to close a PR with good work in it like this one. But you never know when a seemingly straightforward+good issue is actually not ready to be accepted yet.

Thanks! ❤️

@james-pre
Copy link
Author

james-pre commented Dec 23, 2024

@JoshuaKGoldberg All of the tests passed (actions workflow), so I don't believe the instability issues mentioned in the comment are applicable. What do you think?

@JoshuaKGoldberg
Copy link
Member

(answering in #10533)

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 86.84%. Comparing base (4747299) to head (42eb0b2).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...ipt-estree/src/parseSettings/resolveProjectList.ts 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main   #10544   +/-   ##
=======================================
  Coverage   86.83%   86.84%           
=======================================
  Files         445      445           
  Lines       15424    15428    +4     
  Branches     4497     4497           
=======================================
+ Hits        13394    13398    +4     
  Misses       1675     1675           
  Partials      355      355           
Flag Coverage Δ
unittest 86.84% <83.33%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
...ipt-estree/src/parseSettings/resolveProjectList.ts 97.82% <83.33%> (+0.04%) ⬆️

... and 2 files with indirect coverage changes

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 1, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhancement: replace fast-glob with tinyglobby
3 participants