-
Notifications
You must be signed in to change notification settings - Fork 124
fix: respect scoped registries from publishConfig #844
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
fix: respect scoped registries from publishConfig #844
Conversation
This stops a niche issue when using Gitlab registries, where setting the `.npmrc` to pull packages from the instance-wide registry while also setting the `package.json:publishConfig` to push to the project-local registry for a given `@scope` would pass the incorrect `--registry` flag to `npm publish` as well as associate the `NPM_TOKEN` with the wrong URL in the generated userconfig file. This DOES NOT resolve semantic-release#219. So `--registry` is still technically ignored for scoped packages. See [npm/cli#7043](npm/cli#7043) for more details on that. I made sure not to affect the priority of other resolutions to avoid any breaking changes. An additional test has been added for this specific case.
Could you please provide a public reproduction of the problem? What you describe is the default npm behavior, which we do or best to align with already by mostly deferring to npm to handle. I've used such configuration myself without complication, so I'm skeptical of this being a problem
Thanks for your time, @travi!
I don't have a way to reproduce publicly right now, as this is happening in a private self-hosted instance of Gitlab. But I can go into a little more detail.
To define NPM's behavior, assuming a package called @demo/package...
Scopeless
// package.json { "publishConfig": { "registry": "https://pkg.fake.io" } }
# .npmrc
registry=https://rc.fake.io# Command npm publish $(pwd) --registry "https://cli.fake.io" # Attempts to publish to # https://cli.fake.io
Scoped in the .npmrc
// package.json { "publishConfig": { "registry": "https://pkg.fake.io" } }
# .npmrc
@demo:registry=https://rc.fake.io# Command npm publish $(pwd) --registry "https://cli.fake.io" # Attempts to publish to # https://rc.fake.io
Scoped in Both .npmrc and publishConfig
// package.json { "publishConfig": { "@demo:registry": "https://pkg.fake.io" } }
# .npmrc
@demo:registry=https://rc.fake.io# Command npm publish $(pwd) --registry "https://cli.fake.io" # Attempts to publish to # https://pkg.fake.io
This plugin currently only discovers the register field of publishConfig, but it will never recognize a @scope:registry field. Meanwhile, if you look at the changeset, you'll see it does locate scoped registry entries in the project's .npmrc.
So there's no issue in the first two cases above, but on the last case - "Scoped in Both .npmrc and publishConfig" - this plugin deviates from NPM's behavior. When it sets up the userconfig, it will call getRegistry, which will fail to find @demo:registry in the package.json:publishConfig. Instead, it will return the scoped registry from .npmrc and then set up the environment's NPM_TOKEN for that environment instead of the registry npm publish will actually use and authorization will fail when attempting to push.
This change just adds the ability to locate those scoped registries in publishConfig and have them override the entries in .npmrc.
EDIT: In the interim, I've added a separate step to my Gitlab CI/CD to run npm config set -L project just before any semantic release commands (having dependency installation handled by a job in an earlier stage).
Another way to confirm this change is needed is just to reset the changes to lib/get-registry.js and the run the new test:
[test:unit ] get-registry › Get the registry configured in "publishConfig" for scoped package [test:unit ] [test:unit ] test/get-registry.test.js:47 [test:unit ] [test:unit ] 46: [test:unit ] 47: t.is( [test:unit ] 48: getRegistry( [test:unit ] [test:unit ] Difference (- actual, + expected): [test:unit ] [test:unit ] - 'https://custom6.registry.com' [test:unit ] + 'https://custom5.registry.com' [test:unit ] [test:unit ] › file://test/get-registry.test.js:47:5
It's unable to see the scoped registry under publishConfig.
@travi Would you happen to have some time you could look at this?
i'm still unconvinced this is necessary. please help me understand why you would ever need to define a scoped registry specifically under publishConfig. publishConfig is already scoped to the context of the package being published, so it will only ever impact the current package. the scoping then already depends on the name of the package.
why does publishConfig.registry not already solve this usecase without us needing to add special handing for a scoped registry?
why does
publishConfig.registrynot already solve this usecase without us needing to add special handing for a scoped registry?
Gitlab's package registry requires you to use:
- The group registry endpoint for downloading private packages.
- The project registry endpoint to publish your project.
- The default NPM registry for downloading external packages.
So in a project that both uses another internal project and publishes itself, I need:
- An
.npmrcthat contains a@scopedregistry for the group. It cannot be unscoped or it will attempt to use Gitlab for all NPM packages. - A
publishConfigthat points to the registry for the project.
The problem is that publishConfig.registry is completely ignored if .npmrc contains a more specific scoped registry (see the output in the demos above). The only way to set a different publishing endpoint from the downloading endpoint is to use publishConfig[@scoped:registry] instead. But this project does not support publishConfig[@scoped:registry].
Hope that clears it up!
I've also gone into this here, though being an NPM thread that's less focused on the behavior of this project.
I do think that, aside from this being a requirement for this plugin to work correctly in my specific context, it's also just generally a good practice to be compatible with NPM or explicitly disclose we don't support a feature. NPM's publishConfig supports and prioritizes @scope:registry over registry alone, and folk will try to use it. Given it's only 9 line changes and one extra test, it seems like the maintenance burden is worth it in this case.
just generally a good practice to be compatible with NPM
this is a goal of ours, but clearly we have some gaps. i am hesitant to add more complexity to our implementation in order to handle rare cases, because the more paths we implement, the more opportunities there are for us to be out of sync with npm. this is a big part of why i'm asking the questions that i am in this thread.
to be open, i would be far more open to accepting an update that replaced our implementation with the actual implementation that npm uses to resolve these details. since the npm cli was decomposed into smaller packages within the last few years, i'm curious if that logic is exposed in a way that we could just consume it directly. would you be willing to investigate this?
would you be willing to investigate this?
Yeah, of course! Unfortunately, this path is still pretty complex in NPM and I don't think it'd be a particularly helpful approach.
The gist of it is:
- Their base command class loads in a
Config. - The project configuration then flattens itself with a specialized flattening function.
- This is used to generate a manifest.
- Then that manifest is resolved.
- So that it can finally be passed to
pickRegistry.
It would be nice if we could abstract this by calling npm config with execa, but that doesn't respect publishConfig overrides1 .
With that, I still think the PR as-is is the easiest and cleanest way to support this feature.
Footnotes
-
npm config lswill have a small section at the end disclosing differingpublishConfigsettings, but the--jsoninterface won't. ↩
muratgozel
commented
Nov 11, 2024
hi there, i spent hours checking my gitlab token, environment and other stuff that i thought related but looks like it was semantic-release. in gitlab, you can't pull + publish packages if they are under same main group. (scope in npm terms). the messages above explain the issue very clear and im hoping to see semantic-release be fully compatible with gitlab too. (because its a great tool) i will continue to use github until then.
@muratgozel Thanks for the report!
If you'd still like to use Gitlab, there's a workaround where you can run npm config -L project @scope:registry url before Semantic Release runs and after npm install. This will inject the value into .npmrc, which this plugin is able to read. Just make sure that the package.json does not have publishConfig.registry or this plugin will attempt to use it instead (but publishConfig[@scope:registry] is fine).
@travi Any chance you've reconsidered accepting this PR given the feedback and research?
i appreciate the additional research and information. this is on my list to revist and consider more thoroughly, but i cannot promise a timeline at this point
Uh oh!
There was an error while loading. Please reload this page.
This stops a niche issue when using Gitlab registries, where setting the
.npmrcto pull packages from the instance-wide registry while also setting thepackage.json:publishConfigto push to the project-local registry for a given@scopewould(削除) pass the incorrectassociate the--registryflag tonpm publishas well as (削除ここまで)NPM_TOKENwith the wrong URL in the generated userconfig file.This DOES NOT resolve #219. So
--registryis still technically ignored for scoped packages. Seenpm/cli#7043 for more details on that.
I made sure not to affect the priority of other resolutions to avoid any breaking changes. An additional test has been added for this specific case.
EDIT: Fixed description to make it clear I did not fix #219.