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

fix: respect scoped registries from publishConfig #844

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Kenneth-Sills
Copy link

@Kenneth-Sills Kenneth-Sills commented Jul 30, 2024

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 #219. So --registry is still technically ignored for scoped packages. See
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.

EDIT: Fixed description to make it clear I did not fix #219.

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.
@travi
Copy link
Member

travi commented Jul 30, 2024

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

@Kenneth-Sills
Copy link
Author

Kenneth-Sills commented Jul 30, 2024

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).

@Kenneth-Sills
Copy link
Author

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.

@Kenneth-Sills
Copy link
Author

@travi Would you happen to have some time you could look at this?

@travi
Copy link
Member

travi commented Nov 8, 2024

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?

@Kenneth-Sills
Copy link
Author

why does publishConfig.registry not 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 .npmrc that contains a @scoped registry for the group. It cannot be unscoped or it will attempt to use Gitlab for all NPM packages.
  • A publishConfig that 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.

@travi
Copy link
Member

travi commented Nov 8, 2024

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?

@Kenneth-Sills
Copy link
Author

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 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

  1. npm config ls will have a small section at the end disclosing differing publishConfig settings, but the --json interface won't.

@muratgozel
Copy link

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.

@Kenneth-Sills
Copy link
Author

@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).

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.

Use CLI --@scope:registry flag to override scoped registry defined in .npmrc
3 participants