-
Couldn't load subscription status.
- Fork 70
chore: require node >=16 #151
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
base: master
Are you sure you want to change the base?
Conversation
|
This is really good. |
fc785ee to
bfbf2bb
Compare
| const SLASHES_REGEX = /[\\/]/g; | ||
| export function convertSlashes(path: string, separator: PathSeparator) { | ||
| return path.replace(SLASHES_REGEX, separator); | ||
| return path.replaceAll(SLASHES_REGEX, separator); |
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.
What's the benefit of using replaceAll over replace when using RegExp?
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.
it requires the regex to have the g flag and otherwise throws. useful so that 1) the intentions of replacing more than one entry are clear without needing to look at the regex that could be defined somewhere else 2) it makes forgetting the flag throw an error
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.
I'd much rather we check if AbortController is available and use it only when it is than introduce a breaking change. This change will prevent some libraries from using fdir and I don't think that's a good trade-off as the slightly more modern syntax doesn't have any real benefit compared to the cost of users not being able to use the library. E.g. 75lb/file-set#7 (comment)
|
i don't think |
|
let's at least set |
|
@SuperchupuDev @benmccann let's put this on hold. I have removed AbortController in favor of a custom alternative here: #153 |
|
marking as draft until you decide to drop node 14 👍 |
Note
Since node 12 support was restored, this is getting marked as draft indefinitely until it is decided to drop node 14. As such the text below is obsolete
Since the creation of #116,
fdirstarted usingAbortControllerin 2f6145e.AbortControllerwas unflagged in node15.6.0. This would have broken users, but it looks like absolutely no one actually used versions older than that, since I've seen no reports complaining and it's been a whole month. (Someone did mention it after I asked them but apparently they just updated their node version and moved on).According to @thecodrr's comment it can be deduced that the minimum version set in the
enginesfield should be tested in CI. Testing on node 14 was dropped in 3c608bd (a year and a half ago!). And node 16 is already being tested in CI.I could set the field to
>=15.6.0, but I don't think it would be a good idea considering it's not a good practice to set the minimum version to an odd numbered release. So I propose setting it to>=16.0.0, just like the original comment from #116 suggested. This wouldn't break anyone that wasn't already broken by the6.4.5release (which, as I've said before, is either no one or extremely few people, considering neitherfdirortinyglobbygot any reports of it happening).Doing this is also good for a few more reasons. It can allow the package to use a few syntax features such as optional chaining which can make code smaller and more readable while doing the same thing. Also it can make
fdiruse thenode:protocol where possible which is a good practice the node team has been wanting people to use. This is why the PR makes use of those features.A few days ago I opened a pinned issue in
tinyglobbymentioning the possibility of the same happening there. It got two positive reactions and no negative comments, so it seems the reception wouldn't be negative SuperchupuDev/tinyglobby#134Be mindful that if this is merged, #147 would probably need to be updated (by changing the target and removing
removeNodeProtocolfrom the tsdown config). Just mentioning this so that both aren't merged at the same time.closes #116