-
Notifications
You must be signed in to change notification settings - Fork 49
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
[dependencies-replacement] Replace fast-glob with tinyglobby #232
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 964ccdc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The chagned code doesn't have any tests attached to it. I am worried about changed behavior, possibly on Windows. Not sure how best to test it - maybe add a Windows CI path? Or, is Windows generally considered not supported? |
ff21b0b
to
964ccdc
Compare
A CI job for windows would be appreciated regardless - it might be good to PR it separately from this. I'm similarly worried about the changed behavior but as long as this is a widely used package and since we'll be releasing a new 0.x minor with those changes... it should probably be fine. |
|
"jju": "^1.4.0", | ||
"js-yaml": "^4.1.0" | ||
"js-yaml": "^4.1.0", | ||
"tinyglobby": "^0.2.9" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, this change actually touches @manypkg/tools
and not the CLI itself - that makes me way more worried about this change since tinyglobby
might slightly differ in some cases from fast-glob
we might have to consider releasing v2 of this package some time in the future to support the upcoming Changesets v3 release, so maybe that would be the best time to revisit this? wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was gonna raise that this change requires releasing new versions of @manypkg/tools
and @manypkg/get-packages
.
This change does lower the dependency count substantially (by the time everything else merges, it would be by about 30%). However I entirely understand that you don't want it rushed, so just let me know how to best support its eventual merge.
References #221
Removes 15 dependencies from the graph.