-
Notifications
You must be signed in to change notification settings - Fork 115
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
base: master
Are you sure you want to change the base?
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 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
|
Another way to confirm this change is needed is just to reset the changes to [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 |
@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 why does |
Gitlab's package registry requires you to use:
So in a project that both uses another internal project and publishes itself, I need:
The problem is that 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 |
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? |
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:
It would be nice if we could abstract this by calling With that, I still think the PR as-is is the easiest and cleanest way to support this feature. Footnotes
|
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 |
This stops a niche issue when using Gitlab registries, where setting the
.npmrc
to pull packages from the instance-wide registry while also setting thepackage.json:publishConfig
to push to the project-local registry for a given@scope
wouldpass the incorrectassociate the--registry
flag tonpm publish
as well asNPM_TOKEN
with the wrong URL in the generated userconfig file.This DOES NOT resolve #219. So
--registry
is 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.