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

Update to semver ≥ 7 (Fixes: #2005) #2006

Closed
wants to merge 1 commit into from
Closed

Conversation

guimard
Copy link
Contributor

@guimard guimard commented Dec 29, 2019

Description of change

This PR updates semver to 7.x. and fixes #2005

@guimard guimard mentioned this pull request Dec 29, 2019
@rvagg
Copy link
Member

rvagg commented Dec 30, 2019

I'm not keen on this, our package.json has been optimised to fit nicely with npm's so that there's a lot of deduplication. Both node-gyp and npm currently have ^5.7.1 (https://github.com/npm/cli/blob/797a59797a2e226631f068c912f351ba340581fe/package.json#L130). I don't understand the need to push it to 7 or why this lack of support for 7 is a problem for anyone, as I commented in #2005.

@guimard
Copy link
Contributor Author

guimard commented Dec 30, 2019

Hi @rvagg,

I'm Debian Developer. For security reason, our policy does not accept embedded code. That's why there is only one semver in our distribution. I posted here my patch published in our package.

Cheers,
Xavier

@rvagg
Copy link
Member

rvagg commented Dec 30, 2019

(Following on from discussion in #2006)

The new should work with both 5 and 7 and you're going to force semver@7 when you package this upstream anyway, whether we change package.json or not.

So back out the package.json change, leaving us with semver@5, and I'll lift my objection. Without anyone else objecting we should be able to merge this (with a bit of manual testing). We'll bump to semver@7 when npm does--which may be sooner than later with the retirement of Node.js 8 (i.e. a lot of npm's dependencies have moved to support newer JS features that aren't supported by Node.js 8).

I'll +1 this change since it's trivial, but it's not an endorsement (from me at least) of dancing around the incompatible bundling policies of Debian et. al.. That's something you either have to continue to manually patch your way around or push for some policy leniency for runtime ecosystems such as Node.js, whose dependency systems are incompatible with the existing distro policies.

@guimard
Copy link
Contributor Author

guimard commented Dec 30, 2019

(Following on from discussion in #2006)

The new should work with both 5 and 7 and you're going to force semver@7 when you package this upstream anyway, whether we change package.json or not.

So back out the package.json change, leaving us with semver@5, and I'll lift my objection. Without anyone else objecting we should be able to merge this (with a bit of manual testing). We'll bump to semver@7 when npm does--which may be sooner than later with the retirement of Node.js 8 (i.e. a lot of npm's dependencies have moved to support newer JS features that aren't supported by Node.js 8).

I'll +1 this change since it's trivial, but it's not an endorsement (from me at least) of dancing around the incompatible bundling policies of Debian et. al.. That's something you either have to continue to manually patch your way around or push for some policy leniency for runtime ecosystems such as Node.js, whose dependency systems are incompatible with the existing distro policies.

Done, thanks.

Of course, Debian Developer will continue to manually patch the node.js module they need 😉

rvagg pushed a commit that referenced this pull request Jan 6, 2020
Fixes: #2005
Reviewed-By: Rod Vagg <rod@vagg.org>
PR-URL: #2006
@rvagg
Copy link
Member

rvagg commented Jan 6, 2020

landed f242ce4

@rvagg rvagg closed this Jan 6, 2020
rvagg pushed a commit that referenced this pull request Jan 6, 2020
Fixes: #2005
Reviewed-By: Rod Vagg <rod@vagg.org>
PR-URL: #2006
rvagg pushed a commit that referenced this pull request Feb 3, 2020
Fixes: #2005
Reviewed-By: Rod Vagg <rod@vagg.org>
PR-URL: #2006
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update to semver ≥ 7
3 participants