-
Notifications
You must be signed in to change notification settings - Fork 240
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
Create 0002-no-caret-prerelease-installs.md #14
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I appreciate this RFC. I think prerelease versions have historically been a huge footgun and this seems to be a direction to go if we want to make it hurt a little less. I'm unclear about how it would interact with other parts of the system, what the escape hatches are, if any, and why you think this wouldn't be better served by a change in how semver specifies these, since that's ultimately the source of this pain. (Not that I think modifying semver is a better solution but I think exploring that idea and including the conclusion in this RFC would help immensely with context)
0002-no-caret-prerelease-installs.md
Outdated
|
||
## Detailed Explanation | ||
|
||
`npm install`, when writing to `package.json` (like with `--save` or `--save-dev`), should check with semver and only add the `^` if the version isn't a prerelease. I don't believe it should affect anything else. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do users opt into using ranges for prerelease, if they'd like to? This feature is currently controlled by the --save-prefix
config, and this RFC makes no mention of how these changes will interact with that option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I didn't know about that option, wasn't really sure what to put for this section actually!
As a consumer I would be quite surprised if you included breaking changes in a pre-release that wasn't a major version above the current non-prerelease? But you're right, nothing says in that case that you'd be wrong to break from pre-release to pre-release… Something I'd like to figure out for this: How many packages have pre-release range specifiers currently matching multiple versions? How many projects are expecting the current behavior? |
Yeah this is mostly in the case of Babel right now where the current We are publishing
Yeah, that's a good question! I think people would like it since you don't have to continually update but that assumes the non-breaking in-between pre-releases. I think many projects use pre-release as an opportunity to do breaking changes though. Or hmm that's interesting! Maybe specifically for a new major release (so only for a Although I realize it's probably that we aren't using |
fwiw i do generally find it surprising when the prerelease line (alpha/beta/etc) has breaking changes inside the same line. |
Yeah, I guess this is specifically for when doing a major bump and you just want to get some testing out there but clearly not stable yet. I think it's pretty common for breaking changes in alpha/beta, and sometimes RC for some projects. Like see RxJS, others? I guess I don't see how a package author would do it otherwise if they are releasing and landing different kinds of changes before doing the final release? Edit: looks like |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After some discussion @zkat and I are ok with this. This RFC will need an implementation section to land. I believe the implementation will be just the addition of another conditional to https://github.com/npm/cli/blob/latest/lib/install/deps.js#L303
Please note that the implementation section does not need an actual implementation, just a discussion of how it would be done.
Something interesting might be something similar to semver after the
|
@hzoo hey Henry, just a heads-up that this is ready to land, but we need a small implementation section in order to ratify it. Let us know when that happens! Thanks for your time and patience :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the implementation section, @hzoo! The team just met and decided we're ready to ratify this. Thanks again for sending this RFC our way and I hope the implementation makes you and yours lives easier! 🎉
```diff | ||
"devDependencies": { | ||
- "@babel/register": "^7.0.0-beta.52" | ||
+ "@babel/register": "7.0.0-beta.52" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a bit odd that it says @babel/register
here when it's @babel/core
in the install command above 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lol good point, feel free to PR or I can later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can do, simple enough. Just wasn't sure if it was worth a PR
EDIT: #31
Rendered: https://github.com/hzoo/rfcs-1/blob/patch-2/0002-no-caret-prerelease-installs.md
Example gissues related to this:
"decoratorsLegacy": true
option to @babel/preset-stage-2 vuejs/vue-cli#1162 (another issue is also with integrations that may use prerelease versions with^
but that might be a different issue)There is a lot of confusion with prereleases in general (both from the package author and user side), so this doesn't prevent anyone from using
^
but it at least prevents people from accidentally adding it^
given the default.Might even suggest people get warned of keeping the
^
somehow but maybe that's too much.