Skip to content
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

bump glob to v10 #658

Merged
merged 2 commits into from
Mar 28, 2024
Merged

bump glob to v10 #658

merged 2 commits into from
Mar 28, 2024

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Mar 28, 2024

closes #638

sort paths since they come back in reverse order now

sort paths since they come back in reverse order now
Copy link

codecov bot commented Mar 28, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.45%. Comparing base (53c9a54) to head (754b225).

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #658      +/-   ##
==========================================
- Coverage   99.45%   99.45%   -0.01%     
==========================================
  Files          12       12              
  Lines        1467     1465       -2     
  Branches      277      277              
==========================================
- Hits         1459     1457       -2     
  Misses          8        8              

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

@mmkal mmkal merged commit cbfaadb into main Mar 28, 2024
8 checks passed
@mmkal mmkal deleted the glob-v10-again branch March 28, 2024 01:13
@WikiRik
Copy link
Member

WikiRik commented Mar 28, 2024

@mmkal Glob v10 does not support Node 12 as per the following;
https://github.com/isaacs/node-glob/blob/b5d0f640893eba729bb8675a51a73ffbc0760b35/package.json#L96

Are you sure the tests on Node 12 passed?

@mmkal
Copy link
Contributor Author

mmkal commented Mar 28, 2024

It looks like there's a bug that's making the Node 12 and Node 14 npm test commands pass even though they're both failing:

image

I will look into that, and maybe just switch to fast-glob which supports nodejs 8+.

@B4nan
Copy link

B4nan commented Mar 30, 2024

Why would you care about this? Ship a major version if the current one is supposed to work with this. Supporting node 16 is questionable (but understandable), supporting anything older is (IMO) just weird and a waste of (your/maintainer's) time.

Node 12 is not receiving security updates for 2 years now, node 14 for a year, and node 16 for more than 6 months.

@WikiRik
Copy link
Member

WikiRik commented Mar 30, 2024

Why would you care about this? Ship a major version if the current one is supposed to work with this.

If we are shipping this in a major version then it's not an issue and we can use glob v10 just fine. But as far as I know this was supposed to be released as a minor or patch version, which would break older projects and not be in the spirit of semantic versioning.

@mmkal
Copy link
Contributor Author

mmkal commented Mar 30, 2024

I prefer not to release major versions unless there's a good reason. Especially for a library like this which is sometimes a couple layers deep in dependency trees. Using fast-glob is an easy fix, and not worth a major version IMO. The tests for lower node versions were just broken.

Having said that, eventually it will be too much of a headache to support old node versions. At that point I'm fine with a major bump.

Copy link

github-actions bot commented Apr 4, 2024

Released in v3.8.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Snyk find issue in umzug@3.4.0 > glob@8.1.0 > inflight@1.0.6
3 participants