use package manager to get info #85

amclin · 2020-05-01T23:45:48Z

feat: use package manager to find peerDependencies

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

This also fixes the failing pnpm build matrix, allowing the dependabot setup to resume normal operations

Renames onlyPeers parameter to noRegistry which better reflects what this function is doing. This function doesn't care about peers vs. main, that's in a different scope.

… lists `--only-peers` was doing two different things which complicated the need for proxy settings. Not only was it flagging whether to install the main package with the peers, it was also toggling whether the peerDeps list was being generated from a local node_modules vs a remote registry. By splitting this into two distinct feature flags, we make behavior clearer, and future case of delegating proxy settings to NPM or Yarn instead of handling those in the utility. BREAKING_CHANGE: `--only-peers` now searches for peerDeps via the registry unless `--no-registry` is also specified * was: `install-peerdeps <package> --only-peers` * now: `install-peerdeps <package> --only-peers --no-registry`

Errors are often on stderr and that's important for debugging

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

amclin · 2020-05-02T02:49:57Z

This is done and ready for review. This includes the work from #84

The failing build looks like it's something weird with the Node 8 agent, possibly because it doesn't have pnpm

Considering this is a major version bump, it would be an appropriate time to take advantage of the breaking change to drop Node 8.

src/cli.test.js

src/install-peerdeps.js

Co-authored-by: Jordan Harband <ljharb@gmail.com>

ljharb · 2020-05-03T20:08:30Z

.travis.yml

@@ -2,7 +2,6 @@ language: node_js
 node_js:
  - "12"
  - "10"
-  - "8"


To be very explicit: no PR with this change can land, as long as I have a say.

Then it's nothing is going to merge. Because without it is failing and master is failing.

Take it or leave it, your attitude is not one that inspires me to contribute further.

Since node v8 is end of life now, can we merge this? This would be a great improvement for this tool IMO.

A node version being EOL is irrelevant; package authors can and should support engine versions long after their vendors have stopped doing so.

package authors can and should support engine versions long after their vendors have stopped doing so

Ok, I guess. This seems like a very strange hill to die on to me, though. I think not alienating contributors and enabling a whole world of new use cases beats sacrificing your software on the altar of backwards compatibility. 🤷‍♂️

For our team, this contribution would have made install-peerdeps go from an unusable tool to something really useful!

Thanks for the contribution @amclin! We're looking forward to the day this can be released 😄

@ljharb since you refuse to accept a backwards-breaking change, the responsibility is on you to fix master, so that contributions can be made.

Right now, no-one can contribute to this project because master is failing.

This PR is now 9 months old, because master is failing.

Even your automated security patches from Dependabot are not merging, because master is failing.

Fix master, and I'm happy to update this PR to reflect whatever changes you make to master. But no-one else is going to fix master for you, especially since you've made it clear you won't accept others' logical approaches to doing so.

It's totally fair to hold off updating this PR until master is fixed, but if you had restored node 8 to the testing matrix, and the only failures on this PR were the same ones that were already in master, then I'd consider your PR ready to merge. This is a useful change, and I'd prefer to see it go in without disrupting existing users, and without needing to debate support philosophies.

0831778 should have fixed master, so I've rebased this PR (and restored node 8).

ljharb

LGTM, but as-is, it's still a breaking change.

If we can preserve the -r/-a/-p commands, and just render them unnecessary with the default behavior, then this is semver-minor.

ljharb · 2021-01-14T22:32:05Z

README.md

-  -r, --registry <uri>             Install from custom registry (defaults to NPM registry)
+  -n, --no-registry                Do not use a remote registry to find dependencies list
  --dry-run                        Do not install packages, but show the install command that will be run
-  -a, --auth <token>               Provide an NPM authToken for private packages.
-  -p, --proxy <http_proxy>         Enable http proxy to connect to the registry


is there a way we can keep this functionality? the default can (should) still be the behavior in this PR, and -n is a good addition regardless.

I'm not following, that's the point of this PR - remove the custom proxy logic that is unnecessary and doesn't work with many proxy configurations, offloading it to the package manager instead (see my comment on #72). If you have to deal with proxies, you are already having to deal with them in the package manager. Making users deal with them twice is unnecessary pain. I can't have the new behavior as default, while preserve the old behavior in a non-breaking way, because the old behavior was inconsistent with multiple behaviors hidden behind a single flag, (see #84).

Agreed that it's a breaking change and that's why it is flagged as such. Semantic-release should appropriately bump a major version number by design. For more explanation on the breaking change specifically, see #84, which is why I opened this as 2 PRs as this one depends on that one.

Gotcha, that makes sense.

ljharb

LGTM, but should be coordinated with #84 as well.

nathanhleung · 2021-01-20T15:29:35Z

Thank you, this is great! Will release this in v3.

nathanhleung · 2021-01-20T15:41:42Z

Just released v3.0.0!

Available on NPM now too.

amclin added 6 commits May 1, 2020 11:48

refactor: move package install string generation to dedicated method

205a75b

refactor: normalize obj notation for getPackageJson parameters

194e995

Renames onlyPeers parameter to noRegistry which better reflects what this function is doing. This function doesn't care about peers vs. main, that's in a different scope.

refactor: shell spawning needs to be able to capture output

fb51e45

test: improve error reporting from CLI tests

d796ee7

Errors are often on stderr and that's important for debugging

amclin changed the title ~~WIP: use package manager to get info~~ use package manager to get info May 2, 2020

amclin mentioned this pull request May 2, 2020

Add a feature flag to split remote registry from onlyPeers flag #84

Closed

test: improve regex matching for checking --save-dev flags

fd9a985

ljharb reviewed May 2, 2020

View reviewed changes

src/cli.test.js Outdated Show resolved Hide resolved

src/cli.test.js Outdated Show resolved Hide resolved

src/cli.test.js Show resolved Hide resolved

src/install-peerdeps.js Show resolved Hide resolved

src/install-peerdeps.js Show resolved Hide resolved

refactor: use type-safe String() instead of .toString()

d6ed84b

Co-authored-by: Jordan Harband <ljharb@gmail.com>

ljharb requested changes May 3, 2020

View reviewed changes

ljharb mentioned this pull request May 11, 2020

v1.9.0 had a breaking change #80

Open

ljharb reviewed Jan 14, 2021

View reviewed changes

ljharb requested a review from nathanhleung January 15, 2021 01:05

ljharb approved these changes Jan 15, 2021

View reviewed changes

nathanhleung approved these changes Jan 20, 2021

View reviewed changes

nathanhleung merged commit 516a3c6 into nathanhleung:master Jan 20, 2021

amclin deleted the feat/use-package-manager-to-get-info branch January 26, 2021 20:02

This was referenced Jan 27, 2021

fix: add .cmd to commands on windows #124

Merged

Add yarn v2 support #65

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use package manager to get info #85

use package manager to get info #85

amclin commented May 1, 2020 •

edited

Loading

amclin commented May 2, 2020 •

edited

Loading

ljharb May 3, 2020

amclin May 3, 2020

chadly Dec 22, 2020

ljharb Dec 22, 2020

chadly Dec 22, 2020 •

edited

Loading

stevenschmatz Jan 14, 2021

amclin Jan 14, 2021

ljharb Jan 14, 2021 •

edited

Loading

ljharb left a comment

ljharb Jan 14, 2021

amclin Jan 14, 2021 •

edited

Loading

ljharb Jan 15, 2021

ljharb left a comment

nathanhleung commented Jan 20, 2021

nathanhleung commented Jan 20, 2021

use package manager to get info #85

use package manager to get info #85

Conversation

amclin commented May 1, 2020 • edited Loading

amclin commented May 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadly Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amclin Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ljharb left a comment

Choose a reason for hiding this comment

nathanhleung commented Jan 20, 2021

nathanhleung commented Jan 20, 2021

amclin commented May 1, 2020 •

edited

Loading

amclin commented May 2, 2020 •

edited

Loading

chadly Dec 22, 2020 •

edited

Loading

ljharb Jan 14, 2021 •

edited

Loading

amclin Jan 14, 2021 •

edited

Loading