Skip to content

feat: support read scoped registry from package json - #1470

Merged
Andarist merged 7 commits into
changesets:mainfrom
JounQin:feat/scope-registry
Apr 12, 2025
Merged

Andarist merged 7 commits into
changesets:mainfrom
JounQin:feat/scope-registry

Conversation

@JounQin

@JounQin JounQin commented Sep 25, 2024

Copy link
Copy Markdown
Contributor

close #1469

@changeset-bot

changeset-bot Bot commented Sep 25, 2024 •

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fdf8084

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@changesets/cli Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov

codecov Bot commented Sep 25, 2024 •

Copy link
Copy Markdown

Codecov Report

Attention: Patch coverage is 0% with 22 lines in your changes missing coverage. Please review.

Project coverage is 80.66%. Comparing base (dc83cb4) to head (fdf8084).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
packages/cli/src/commands/publish/npm-utils.ts 0.00% 21 Missing ⚠️
...ckages/cli/src/commands/publish/publishPackages.ts 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1470      +/-   ##
==========================================
- Coverage   81.16%   80.66%   -0.51%     
==========================================
  Files          54       54              
  Lines        2241     2255      +14     
  Branches      674      681       +7     
==========================================
  Hits         1819     1819              
- Misses        417      431      +14     
  Partials        5        5              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@JounQin

JounQin commented Sep 25, 2024

Copy link
Copy Markdown
Contributor Author

Not sure how to add related test cases.

cc @Andarist

@bluwy bluwy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Seems good to me. Docs for publishConfig['@scope:registry'] is limited, but it looks like it works according to #1272 (and the references within it, which also resulted in this PR?)

@JounQin

JounQin commented Oct 11, 2024

Copy link
Copy Markdown
Contributor Author

@Andarist Any time to review this PR?

@RodrigoHamuy

Copy link
Copy Markdown
Contributor

Alo, just checking when can we merge this as this is an issue for us since we are using yarn and private Github Packages.

@JounQin

JounQin commented Apr 7, 2025

Copy link
Copy Markdown
Contributor Author

friendly ping @Andarist

@Andarist Andarist left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Overall looks good but I left some comments that have to be addressed before we land this.

@JounQin
JounQin requested a review from Andarist April 8, 2025 13:47
@JounQin

JounQin commented Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

@Andarist Any further review?

@Andarist

Copy link
Copy Markdown
Member

@JounQin could you double-check the changes I made in b932a83 ?

? "https://registry.npmjs.org"
: registry;
return {
scope: undefined,

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
scope: undefined,

Given it's optional, we can omit this.

@JounQin

JounQin commented Apr 11, 2025

Copy link
Copy Markdown
Contributor Author

@JounQin could you double-check the changes I made in b932a83 ?

@Andarist Good cleaner refactor, LGTM, just left a unimportant comment/suggestion.

False positive, so make it happy
Comment thread .changeset/eleven-panthers-talk.md Outdated
"@changesets/cli": minor
---

Support scoped registries configured using `package.json#publishConfig` and environment variables

@JounQin JounQin Apr 11, 2025 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think we should mention more specifically what's the format in case that's really not well known, maybe linking to the @npm/cli or #1469 would be better.

const scope = packageName.split("/")[0];
const scopedRegistry =
packageJson!.publishConfig?.[`${scope}:registry`] ||
process.env[`npm_config_${scope}:registry`];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

last check before merging this - have you tried using this env variable? I know for sure it works with packageJson.publichConfig but I have not tested this with the env variable path

@JounQin JounQin Apr 12, 2025 •

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@Andarist It's impossible to set the env directly via cli or envrc or export, because the environment name contains @ and :, but it's possible to set it with exec#env override.

image

Comment thread .changeset/eleven-panthers-talk.md Resolved
@Andarist
Andarist merged commit 29f34a3 into changesets:main Apr 12, 2025
@github-actions github-actions Bot mentioned this pull request Apr 12, 2025
@JounQin
JounQin deleted the feat/scope-registry branch April 12, 2025 09:44
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.

[cli] npm info should respect publishConfig#@private:registry config

5 participants