Skip to content
New issue

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

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

Already on GitHub? # to your account

fix: better handling of whitespace #564

Merged
merged 1 commit into from
Jun 15, 2023
Merged

fix: better handling of whitespace #564

merged 1 commit into from
Jun 15, 2023

Conversation

lukekarrys
Copy link
Contributor

No description provided.

@lukekarrys lukekarrys requested a review from a team as a code owner June 15, 2023 04:04
@lukekarrys lukekarrys force-pushed the lk/whitespace branch 3 times, most recently from 0dcb021 to fc08341 Compare June 15, 2023 05:20
@lukekarrys lukekarrys self-assigned this Jun 15, 2023
@lukekarrys lukekarrys merged commit 717534e into main Jun 15, 2023
@lukekarrys lukekarrys deleted the lk/whitespace branch June 15, 2023 19:21
@github-actions github-actions bot mentioned this pull request Jun 15, 2023
@jportner
Copy link

Snyk reported that this PR fixed a vulnerability in semver 7.5.1: https://security.snyk.io/vuln/SNYK-JS-SEMVER-3247795

According to Snyk, it looks like semver 5.7.1 has the same issue. It seems a lot of packages still depend on semver 5.x. Is there any chance of applying this fix in a 5.7.2 release as well?

(I looked but I didn't see a 5.x branch in this repo)

@SymbioticKilla
Copy link

@lukekarrys @wraithgar
It would be awesome to have this fix in 5.x and 6.x branches
Thank you!

@wraithgar
Copy link
Member

At this time, seeing as how v5/6 is a single 1400+ line file with outdated testing deps and no working CI we do not have plans to backport this.

@joaomoreno
Copy link

@wraithgar, while I totally side with you on this, it's much more difficult to bulk upgrade semver to v7 across deep dependency trees of even medium side projects. We have 10+ usages of it just in microsoft/vscode. This is a compliment: semver is very popular and its usage is widespread. We thank you for this library! ❤️

That being said, we like to put our commits where our mouth is. I've backported the fix to v5: https://github.com/npm/node-semver/compare/v5.7.1...joaomoreno:joao/backport-564-to-v5?expand=1. I've also fixed the test suites and added the relevant test cases, all green: https://asciinema.org/a/593286. I hope you can consider this for releasing an eventual 5.7.2 version, given your review. If successful, I would gladly backport the same fix to v6.

I can't quite create a PR, since there's no v5 branch to upstream against. If you'd like to roll this forward, let's create a v5 branch and we'll create a PR from my ref.

@ljharb
Copy link

ljharb commented Jun 26, 2023

That would be amazing; semver v7's dropping of node versions means i'm stuck on v6 for basically ever on dozens of packages.

@1EDExg0ffyXfTEqdIUAYNZGnCeajIxMWd2vaQeP

The babel team has also stated that they'd fork v6 to fix the issue. babel/babel#15720 (comment)

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

Successfully merging this pull request may close these issues.

10 participants