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

JS Packages: update URL structure #41101

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from
Open

Conversation

kraftbj
Copy link
Contributor

@kraftbj kraftbj commented Jan 15, 2025

When monitoring a npm package autopublish, I noticed this in the logs:

npm warn publish npm auto-corrected some errors in your package.json when publishing.  Please run "npm pkg fix" to address these errors.
npm warn publish errors corrected:
npm warn publish "repository.url" was normalized to "git+https://github.com/Automattic/jetpack.git"

Proposed changes:

  • Run npm pkg fix in js-packages that are auto-published to resolve.
  • Did not run in the other directories to avoid noise.
  • Updated the CLI generate skeleton.

Other information:

  • Have you written new tests for your changes, if applicable?
  • Have you checked the E2E test CI results, and verified that your changes do not break them?
  • Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

none

Does this pull request change what data or activity we track or use?

no

Testing instructions:

  • Ensuring our CLI is happy and monioring the log the next time something autopublishes (post-merge)

@github-actions github-actions bot added [Package] Protect Status [Package] Publicize [Package] Stats Admin [Package] Transport Helper [Package] VideoPress [Package] WP JS Data Sync [Package] Yoast Promo [Plugin] Automattic For Agencies Client [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Classic Theme Helper Plugin [Plugin] CRM Issues about the Jetpack CRM plugin [Plugin] Inspect [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] mu wpcom jetpack-mu-wpcom plugin [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Plugin] Social Issues about the Jetpack Social plugin [Plugin] Starter Plugin [Plugin] Super Cache A fast caching plugin for WordPress. [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Plugin] Wpcomsh [Tests] Includes Tests Actions GitHub actions used to automate some of the work around releases and repository management E2E Tests labels Jan 15, 2025
@kraftbj
Copy link
Contributor Author

kraftbj commented Jan 15, 2025

@tbradsha yup. done!

https://docs.npmjs.com/cli/v11/configuring-npm/package-json#repository

It looks like it does expect git+ for git repo URLs. There are some other conventions we could use (github:automattic/jetpack) and ability to indicate the directory in a monorepo.

@tbradsha
Copy link
Contributor

ability to indicate the directory in a monorepo

I was just reading that doc and was going to mention that but checked...apparently we already use directory. 😄

tbradsha
tbradsha previously approved these changes Jan 15, 2025
Copy link
Contributor

@anomiex anomiex left a 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
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor

@tbradsha tbradsha Jan 15, 2025

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/

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Actions GitHub actions used to automate some of the work around releases and repository management E2E Tests [Feature] Calypsoify [Feature] Contact Form [Feature] Google Analytics [Feature] Masterbar WordPress.com Toolbar and Dashboard customizations [Feature] Publicize Now Jetpack Social, auto-sharing [Feature] Theme Tools [JS Package] AI Client [JS Package] Analytics [JS Package] API [JS Package] Babel Plugin Replace Textdomain [JS Package] Boost Score Api [JS Package] Charts [JS Package] Components [JS Package] Config [JS Package] Connection [JS Package] Critical Css Gen [JS Package] Eslint Changed [JS Package] Eslint Config Target Es [JS Package] I18n Check Webpack Plugin [JS Package] I18n Loader Webpack Plugin [JS Package] Image Guide [JS Package] Jetpack CLI [JS Package] Partner Coupon [JS Package] Publicize Components [JS Package] React Data Sync Client [JS Package] Remove Asset Webpack Plugin [JS Package] Scan [JS Package] Script Data [JS Package] Shared Extension Utils [JS Package] Social Logos [JS Package] Storybook [JS Package] Svelte Data Sync Client [JS Package] Videopress Core [JS Package] Webpack Config [Package] Ad aka WordAds [Package] Admin Ui [Package] Backup [Package] Blaze [Package] Boost Core [Package] Calypsoify [Package] Chatbot [Package] Classic Theme Helper [Package] Connection [Package] Explat [Package] Forms [Package] Google Analytics [Package] Import [Package] Jetpack mu wpcom WordPress.com Features [Package] Jitm [Package] Masterbar [Package] My Jetpack [Package] Plans [Package] Plugin Deactivation [Package] Protect Status [Package] Publicize [Package] Stats Admin [Package] Transport Helper [Package] VideoPress [Package] WP JS Data Sync [Package] Yoast Promo [Plugin] Automattic For Agencies Client [Plugin] Boost A feature to speed up the site and improve performance. [Plugin] Classic Theme Helper Plugin [Plugin] CRM Issues about the Jetpack CRM plugin [Plugin] Inspect [Plugin] Jetpack Issues about the Jetpack plugin. https://wordpress.org/plugins/jetpack/ [Plugin] mu wpcom jetpack-mu-wpcom plugin [Plugin] Protect A plugin with features to protect a site: brute force protection, security scanning, and a WAF. [Plugin] Search A plugin to add an instant search modal to your site to help visitors find content faster. [Plugin] Social Issues about the Jetpack Social plugin [Plugin] Starter Plugin [Plugin] Super Cache A fast caching plugin for WordPress. [Plugin] VideoPress A standalone plugin to add high-quality VideoPress videos to your site. [Plugin] Wpcomsh [Pri] Low Reviewer Can Merge PR author indicates the reviewer is free to merge/deploy if they want to own the change. RNA [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. [Tests] Includes Tests [Tools] Development CLI The tools/cli to assist during JP development. [Type] Janitorial
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants