-
Notifications
You must be signed in to change notification settings - Fork 91
chore(npm): Scope package to @xmldom org #278
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
Conversation
I am starting to have some "dumb" questions: I am assuming that this to be a squash merge commit. The first version under the Contrary to what I have seen discussed elsewhere, I would be a bit reluctant to publish updates for anything pre-0.7.0. I would certainly consider the introduction of package scope to be a breaking change by itself; previous security updates needed breaking changes; impact of breaking changes up to Which brings up another question: if we already have a Shouldn't we consider bumping the version to P.S. I would like to add one more comment for the record about the scope change in general: Considering the disappointing results of issue #271, I think updating the info in At this point https://www.npmjs.com/package/xmldom is pointing to this project, but this could change at any point of time. |
@@ -1,5 +1,5 @@ | |||
{ | |||
"name": "xmldom", | |||
"name": "@xmldom/xmldom", |
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.
👍 now agreed on my part, should maximize consistency with the GitHub project URL and how this project would normally be installed from GitHub: npm:xmldom/xmldom
or npm:xmldom/xmldom#version
(version
could be branch, tag, or commit ID)
I will do a bit of research how other packages are handling this. Since the only change is the package name i would rather consider it a patch version change. I'm also on favor of not updaying the existing 0.7.0 tag. It's true that the argument of unattended update due to version ranges in package.json would not work. But providing the patched versions (which I already prepared), make it easier to just switch to the scoped package, without the need to consider our recent breaking changes. |
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.
LGTM
While I would agree with the notion that this update is very limited in scope, I would still consider it to be a potentially breaking change. If someone would do an install from GitHub, the package name under dependencies would likely change as well ... and the import statement would need to be updated in the downstream source code. |
But I think we could also just (git) tag it as |
And what about the next release? I would downvote the scoped tagging in general. This would add some ugliness to releases from GitHub, installing by tag from GitHub, downloading ZIP of a release from GitHub, etc. etc. |
Changing all the usages in source code could be avoided by putting the following into package.json: I think I'm still more convinced by the idea that the same code being published in different places can happily have the same versions. |
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.
Good to go, thanks @karfau.
I would continue to downvote changing the package name in the patch release, not 100% sure if all recent npm versions will work properly with this. I would favor putting the scoped package name together with PR #279 (removing extra author / maintainer info) in a new 0.8.0
release. But I think my sentiment should not block merging and publishing these updates.
I think renaming the package makes all versioning considerations irrelevant. LGTM, thanks! |
@brodybits a version tag that would be semver compatible would be to add something as the build metadata. Since |
Thanks @karfau. I had thought that npm did not work so well with the suffix starting with "+" ... but if it works, great! I think I understand your proposal after multiple read-throughs. So here is how I think this would play out for a while starting with
I think this means that upgrades from npm will need special handling for a while, and anyone installing from GitHub may also need to do some special handling for a while as well. No objections on my part, just wanted to think it through a little before we move forward. |
Yes, @brodybits this is exactly what I had in mind, thx for elaborating it this precisely. I will link to this comment in the changelog 😉 |
since they are visible on github/npm level and don't need to additionally be synced/maintained inside `package.json`, as discussed for followup in PR #278.
The package is now scoped under @xmldom. See xmldom/xmldom#278
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796
The package is now scoped as @xmldom. See xmldom/xmldom#278 this fixes CVE-2021-32796 https://nvd.nist.gov/vuln/detail/CVE-2021-32796
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto which depends on xmldom.
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto which depends on xmldom.
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto and xml-encryption which depends on xmldom.
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto and xml-encryption which depends on xmldom.
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto and xml-encryption which depends on xmldom.
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto and xml-encryption which depends on xmldom.
The package is now scoped under @xmldom. See xmldom/xmldom#278 This fixes security vulnerability CVE-2021-32796. Also update xml-crypto and xml-encryption which depends on xmldom.
As discussed in the recent days, this PR prepares the scoping of the package into the npm organisation
xmldom
so the new package name is@xmldom/xmldom
.See https://docs.npmjs.com/about-scopes
I also checked that the
@
has to be part of the shields/badges URL to work, by looking at https://shields.io/category/version and trying it with an existing scoped package.