8000 chore(npm): Scope package to @xmldom org by karfau · Pull Request #278 · xmldom/xmldom · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

Merged
merged 5 commits into from
Aug 19, 2021
Merged

Conversation

karfau
Copy link
Member
@karfau karfau commented Aug 19, 2021

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.

@karfau karfau requested review from kethinov and brody4hire August 19, 2021 14:44
@brody4hire
Copy link
Member
brody4hire commented Aug 19, 2021

I am starting to have some "dumb" questions:

I am assuming that this to be a squash merge commit.

The first version under the @xmldom/xmldom scope would be 0.7.0 ... right?

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 0.7.0 seem to be pretty limited.

Which brings up another question: if we already have a 0.7.0 tag without the @xmldom org scope, and people may have already starting using the 0.7.0 tag from GitHub, then what tag should we use on GitHub once we change the scope to @xmldom/xmldom?

Shouldn't we consider bumping the version to 0.8.0 to help avoid potential confusion?


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 readme.md as proposed here was needed to avoid potential confusion between https://github.com/jindw/xmldom, the unscoped xmldom package which we have lost control over, and this project which will now be published as @xmldom/xmldom.

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",
Copy link
Member

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)

@karfau
Copy link
Member Author
karfau commented Aug 19, 2021

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.

Copy link
@joebowbeer joebowbeer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@brody4hire
Copy link
Member

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.

@karfau
Copy link
Member Author
karfau commented Aug 19, 2021

But I think we could also just (git) tag it as @xmldom/xmldom@0.7.0

@brody4hire
Copy link
Member

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.

@karfau
Copy link
Member Author
karfau commented Aug 19, 2021

Changing all the usages in source code could be avoided by putting the following into package.json:
"xmldom":"@xmldom/xmldom@0.7.0" (the version doesn't really matter here)

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.

Copy link
Member
@brody4hire brody4hire left a 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.

@forty
Copy link
Contributor
forty commented Aug 19, 2021

I think renaming the package makes all versioning considerations irrelevant. LGTM, thanks!

@karfau
Copy link
Member Author
karfau commented Aug 19, 2021

@brodybits a version tag that would be semver compatible would be to add something as the build metadata. Since @ and / are not allowed we could just add 0.7.0+unscoped to the same commit that is now 0.7.0 and 0.7.0+scoped to the commit from this PR and document the meaning in the changelog.
In the future I would just use version numbers without any additional metadata, since there is only the scoped version being released.

@brody4hire
Copy link
Member

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 0.6.0, leaving patch releases out of consideration:

  • existing tag 0.6.0 - unscoped xmldom package version 0.6.0, already published to npm
  • existing tag 0.7.0 - unscoped xmldom package version 0.7.0 ... not published to npm, no plan to publish to npm
  • upcoming new tag 0.7.0+unscoped - same as existing tag 0.7.0 above
  • upcoming new tag 0.7.0+scoped - plan to publish to npm as @xmldom/xmldom version 0.7.0
  • future version 0.8.0 with tag 0.8.0 - would be published to npm as @xmldom/xmldom version 0.8.0

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.

@karfau
Copy link
Member Author
karfau commented Aug 19, 2021

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 😉

@karfau karfau merged commit 322c55b into xmldom:master Aug 19, 2021
@karfau karfau deleted the scope-package branch August 19, 2021 19:31
@karfau karfau added this to the 0.7.0 milestone Aug 19, 2021
karfau pushed a commit that referenced this pull request Aug 19, 2021
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.
forty added a commit to forty/xml-crypto that referenced this pull request Aug 19, 2021
The package is now scoped under @xmldom. See xmldom/xmldom#278
forty added a commit to forty/node-xml-encryption that referenced this pull request Aug 19, 2021
The package is now scoped under @xmldom. See xmldom/xmldom#278
This fixes security vulnerability CVE-2021-32796
LoneRifle pushed a commit to node-saml/xml-crypto that referenced this pull request Aug 20, 2021
The package is now scoped under @xmldom. See xmldom/xmldom#278
This fixes security vulnerability CVE-2021-32796
LoneRifle pushed a commit to node-saml/xml-crypto that referenced this pull request Aug 20, 2021
The package is now scoped under @xmldom. See xmldom/xmldom#278
This fixes security vulnerability CVE-2021-32796
forty added a commit to forty/node-saml that referenced this pull request Aug 20, 2021
The package is now scoped under @xmldom. See xmldom/xmldom#278
This fixes security vulnerability CVE-2021-32796
bk-tba added a commit to bk-tba/svg-spritemap-webpack-plugin that referenced this pull request Aug 24, 2021
forty added a commit to forty/passport-saml that referenced this pull request Aug 24, 2021
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.
gkwang pushed a commit to auth0/node-xml-encryption that referenced this pull request Aug 24, 2021
The package is now scoped under @xmldom. See xmldom/xmldom#278
This fixes security vulnerability CVE-2021-32796
forty added a commit to forty/passport-saml that referenced this pull request Aug 25, 2021
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.
forty added a commit to forty/passport-saml that referenced this pull request Aug 25, 2021
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.
forty added a commit to forty/node-saml that referenced this pull request Aug 25, 2021
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.
forty added a commit to forty/node-saml that referenced this pull request Aug 25, 2021
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.
cjbarth pushed a commit to node-saml/passport-saml that referenced this pull request Aug 26, 2021
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.
cjbarth pushed a commit to node-saml/node-saml that referenced this pull request Aug 26, 2021
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.
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.

4 participants
0