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.

Sign up for GitHub

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

Don't coerce version strings to semver, quote package names #126

Merged
merged 2 commits into from
Jan 29, 2021
Merged

Don't coerce version strings to semver, quote package names #126

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
12dfcf8
fix: don't coerce version strings to semver. resolves #71
nathanhleung Jan 29, 2021
fa0b30e
fix: quote package names, resolves #64
nathanhleung Jan 29, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 5 additions & 7 deletions src/install-peerdeps.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,12 +154,9 @@ const getPackageString = ({ name, version }) => {
if (version.indexOf(" ") >= 0) {
// Semver ranges can have a join of comparator sets
// e.g. '^3.0.2 || ^4.0.0' or '>=1.2.7 <1.3.0'
// Take each version in the range and find the maxSatisfying
const rangeSplit = version
.split(" ")
.map(v => coerce(v))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it does seem important to still check range validity here? altho then i suppose dist-tags like “next” wouldn’t work.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the range is invalid wouldn't that be a bug in the other package? Do you know how NPM handles invalid ranges here?

Based on the package.json docs on the NPM website, simply taking the last element of the range should work.

Related PR: #4

Thanks for reviewing so fast and sorry for the messy diff — I think my prettier extension settings have changed since I first started writing this package. Force-pushed without the formatting so it's hopefully easier to read.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah i think it makes the most sense to just let the package manager handle it.

.filter(v => valid(v));
const versionToInstall = maxSatisfying(rangeSplit, version);
// Take the last version in the range
const rangeSplit = version.split(" ");
const versionToInstall = rangeSplit[rangeSplit.length - 1];

if (versionToInstall === null) {
return name;
Expand Down Expand Up @@ -250,7 +247,8 @@ function installPeerDeps(
// Remove -0
return packageName.substr(0, packageName.length - 2);
}
return packageName;
// Fix #64 -- add quotes
return `"${packageName}"`;
}
// If we have spaces in our args spawn()
// cries foul so we'll split the packagesString
Expand Down