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

Why not auto-detect NPM registry from .npmrc? #73

Closed
erhhung opened this issue Feb 27, 2020 · 10 comments · Fixed by #85
Closed

Why not auto-detect NPM registry from .npmrc? #73

erhhung opened this issue Feb 27, 2020 · 10 comments · Fixed by #85

Comments

@erhhung
Copy link

erhhung commented Feb 27, 2020

I see in discussion from #16 that the OP wanted to install dependencies in a package from a private registry, and the subsequent support for that through the --registry option.

My question is, why does install-peerdeps not work alongside npm (or yarn) by using settings that are already set in ~/.npmrc (or ~/.yarnrc), especially with respect to use of a private NPM registry.

For example, I have published eslint-config-myconfig in my private registry, the URL of which I've configured in my ~/.npmrc file. So, if I run this command to query the package's peer dependencies:

$ npm info eslint-config-myconfig peerDependencies --json
{
  "@typescript-eslint/eslint-plugin": ">=2.21 <3",
  "@typescript-eslint/parser": ">=2.21 <3",
  "eslint": ">=6.8 <7",
  "eslint-config-airbnb-typescript": ">=7 <8",
  "eslint-config-prettier": ">=6.10 <7",
  "eslint-plugin-import": ">=2.20 <3",
  "eslint-plugin-prettier": ">=3.1 <4",
  "eslint-plugin-security": ">=1.4 <2",
  "prettier": ">=1.19 <2",
  "typescript": ">=3.8 <4"
}

It just works!

The same should be true when it comes to installing those peer dependencies. When spawning a child process to run npm (or yarn), just let the underlying package manager read its own configuration file.

@ljharb
Copy link
Collaborator

ljharb commented Feb 27, 2020

It could certainly shell out to npm config get registry.

@amclin
Copy link
Contributor

amclin commented Apr 30, 2020

This would be a huge improvement. As far as I can tell, this project right now is unusable because our corporate requirements result in needing to use multiple regestries, which are separated by package namespaces:

Example

  • @my-corp/my-package loads from private registry "A"
  • @my-other-corp-group/my-package loads from private registry "B"
  • eslint loads from a mirror of the public NPMJS.org registry, which is private registry "C"

@ljharb
Copy link
Collaborator

ljharb commented Apr 30, 2020

Hmm, I'm not sure how that would work with npm config get registry, which pulls a single value; if you're using scoped registries, npm install figures that out by itself.

@amclin
Copy link
Contributor

amclin commented Apr 30, 2020

The point of this script is to install peer dependencies for a specified package, right? npm isntall doesn't do that. So if a package has namespaced dependencies, it should be able to resolve those.

Seems this package could do that by just passing each found package to node install via child_process.spawn(). That's effectively what I'm having to build as a custom script instead of using this utility.

@ljharb
Copy link
Collaborator

ljharb commented Apr 30, 2020

That's basically what it's already doing: https://github.com/nathanhleung/install-peerdeps/blob/master/src/install-peerdeps.js#L84-L103

Since it's using npm install by default, in the proper directory, I'm not sure why it's not already working.

I think the issue is that getPackageData is making a direct request to the registry. I think the solution is, to shell out to npm show --json ${packageName}, and use that data - that way the registry for the package will be taken from npmrc normally.

@amclin
Copy link
Contributor

amclin commented Apr 30, 2020

Yeah, since getPackageJson calls getPackageData, and passes the provided registry from the CLI option, it can only track across one registry. There's nothing in the NPM standard mandating dependencies are available in the same registry as the package referencing them.

@ljharb
Copy link
Collaborator

ljharb commented Apr 30, 2020

A PR would be appreciated.

@amclin
Copy link
Contributor

amclin commented May 1, 2020

I'm not sure how to address it in this project without significant restructuring of the interfaces being exposed. In the script I've built privately, I've avoided all the registry and proxy passing entirely (that adds a huge amount of complexity with the corporate proxies I have to deal with). Instead of using the remote registry to get package data or npm show, I'm just drilling into node_modules to find a package.json like the peers-only option does here. That's not particularly future-proof though and is a good design question for what this project wants to do.

@ljharb
Copy link
Collaborator

ljharb commented May 1, 2020

It seems like just a refactor in getPackageJson, you can leave getPackageData intact and unused?

@amclin
Copy link
Contributor

amclin commented May 1, 2020

I'll take a look at branching this and see what I come up.

nathanhleung pushed a commit that referenced this issue Jan 20, 2021
Fixes #73

Removes a bunch of custom logic and leverages the "info"
command that all package managers provide for getting package
details from a remote registry. By using the package tool, we can
delegate proxy configs, private registry paths, and auth tokens
to the package manager. This is especially valuable in networks
that require complex proxy configurations, or when packages
have peerDependencies that may be in multiple other private
than the singular one that was specified

BREAKING_CHANGE: removes `--registry` `--auth` and `--proxy` options

Use `.npmrc` or `.yarnrc` to mange private registry paths, auth tokens, and proxy configs
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 a pull request may close this issue.

3 participants