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

Check save-prefix satisfies requested install version range #193

Closed

Conversation

gosplit
Copy link

@gosplit gosplit commented Apr 26, 2019

The bug report is also opened in npm community.

In short, the recent Angular Compiler requires older typescript packages and asks users to run npm install typescript@">=3.1.1 <3.3"

However, after running the install command, package.json will be update to typescript@^3.2.4, which will still pull incompatible version, eg.'typescript@3.3.0' in other build machine. So I add some checks in computeVersionSpec to ensure adding the save-prefix ^ or ~ only when it satisfies requested version range.

@gosplit gosplit requested a review from a team as a code owner April 26, 2019 09:01
@gosplit gosplit force-pushed the save-prefix-according-range-requested branch from 729c369 to 442a88b Compare May 13, 2019 06:11
@gosplit gosplit force-pushed the save-prefix-according-range-requested branch from 442a88b to 3042e1a Compare May 13, 2019 08:05
@isaacs isaacs added semver:major backwards-incompatible breaking changes needs-discussion labels Jun 26, 2019
@isaacs
Copy link
Contributor

isaacs commented Jun 26, 2019

I think there's probably a better way to do this, but I get that this is a problem. If the user provides a complex range, and save-prefix + resolved version is not a subset of that range, then it's a problem. In that case, I'd prefer to save the range that the user provided, but of course that can run into issues if you do something like npm i foo@'>=1.0.0 or something, because you'll pick up a semver major without realizing it. Maybe that's fine, if that's what they want? This needs some more discussion, though.

@gosplit
Copy link
Author

gosplit commented Jul 3, 2019

I agree with @isaacs. It's better if we can just save the user-provided range in the package.json file.
However, as labeled, this new implementation will break back-compatibility.
To compromise with the back-compatibility, maybe we can just try out [^,~,''] to find the first suitable save-prefix to save?

@isaacs
Copy link
Contributor

isaacs commented Sep 11, 2020

npm/arborist#127

@isaacs isaacs closed this Sep 11, 2020
isaacs added a commit to npm/arborist that referenced this pull request Sep 28, 2020
If a user installs `foo@1.x <1.2.3`, and we resolve to `1.2.2`, then we
should not save it as `^1.2.2`, since that would allow versions outside
of the requested range.

Explicit versions and tags are still saved using the savePrefix, since
those are not ranges, and users can set `--save-exact` if they wish it
to be saved exactly.

Fix: #127
Fix: npm/cli#193
Fix: https://npm.community/t/7005
isaacs added a commit to npm/arborist that referenced this pull request Sep 28, 2020
If a user installs `foo@1.x <1.2.3`, and we resolve to `1.2.2`, then we
should not save it as `^1.2.2`, since that would allow versions outside
of the requested range.

Explicit versions and tags are still saved using the savePrefix, since
those are not ranges, and users can set `--save-exact` if they wish it
to be saved exactly.

Fix: #127
Fix: npm/cli#193
Fix: https://npm.community/t/7005
isaacs added a commit to npm/arborist that referenced this pull request Sep 29, 2020
If a user installs `foo@1.x <1.2.3`, and we resolve to `1.2.2`, then we
should not save it as `^1.2.2`, since that would allow versions outside
of the requested range.

Explicit versions and tags are still saved using the savePrefix, since
those are not ranges, and users can set `--save-exact` if they wish it
to be saved exactly.

Fix: #127
Fix: npm/cli#193
Fix: https://npm.community/t/7005

PR-URL: #145
Credit: @isaacs
Close: #145
Reviewed-by: @isaacs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Discussion is pending a discussion Release 7.x work is associated with a specific npm 7 release semver:major backwards-incompatible breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants