-
Notifications
You must be signed in to change notification settings - Fork 806
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
JS Packages: update URL structure #41101
base: trunk
Are you sure you want to change the base?
Conversation
@tbradsha yup. done! https://docs.npmjs.com/cli/v11/configuring-npm/package-json#repository It looks like it does expect |
I was just reading that doc and was going to mention that but checked...apparently we already use |
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.
I think npm may be being overly strict here.
The URL should be a publicly available (perhaps read-only) URL that can be handed directly to a VCS program without any modification. It should not be a URL to an html project page that you put in your browser. It's for computers.
git clone https://github.com/Automattic/jetpack
appears to work fine.
For that matter, git clone git+https://github.com/Automattic/jetpack.git
doesn't work. 🤷
$ git clone git+https://github.com/Automattic/jetpack.git
Cloning into 'jetpack'...
git: 'remote-git+https' is not a git command. See 'git --help'.
fatal: remote helper 'git+https' aborted session
@@ -165,7 +165,7 @@ for PROJECT in projects/*/*; do | |||
echo "::error file=$PROJECT/package.json${LINE:-$LINE2}::Set \`.repository.type\` to \"git\", as the monorepo is a git repository." | |||
fi | |||
URL="$(jq -r '.url' <<<"$JSON")" | |||
if [[ "$URL" != "https://github.com/Automattic/jetpack.git" && "$URL" != "https://github.com/Automattic/jetpack" ]]; then | |||
if [[ "$URL" != "git+https://github.com/Automattic/jetpack.git" && "$URL" != "https://github.com/Automattic/jetpack" ]]; then |
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.
Possibly the second one should also be changed?
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.
Or maybe remove it entirely, if we really want to match what npm pkg fix
does.
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.
I was assuming .git
suffix would mean git-consumable, whereas without it would be a normal user-friendly URL. But now that you quoted that line and showed that git clone
doesn't work I'm even more confused.
I was trying to track down if/when this changed, but couldn't. I found the example in the documentation changed here:
npm/cli#7615
I did find there's a noGitPlus
option on hosted-git-info
, which seems to be what provides the normalized URL info:
https://github.com/npm/hosted-git-info/
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.
It seems to be something internal where git+https
or git+ssh
but, in either case, it knows it is a github link, so it doesn't look like it actually matters?
I could see where an enterprise version of GH or other self-hosted git might have more of a need for it.
I'm fine dropping this as not needed since npm is figuring it out just fine it seems like. It doesn't really seem to be adding value unless I'm missing something.
When monitoring a npm package autopublish, I noticed this in the logs:
Proposed changes:
npm pkg fix
in js-packages that are auto-published to resolve.Other information:
Jetpack product discussion
none
Does this pull request change what data or activity we track or use?
no
Testing instructions: