-
Notifications
You must be signed in to change notification settings - Fork 945
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
- Loading branch information
Showing
1 changed file
with
1 addition
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ | |
"name": "forever", | ||
"preferGlobal": "true", | ||
"description": "A simple CLI tool for ensuring that a given node script runs continuously (i.e. forever)", | ||
"version": "0.15.3", | ||
"version": "0.15.4", | ||
"author": "Charlie Robbins <[email protected]>", | ||
"maintainers": [ | ||
"Igor Savin <[email protected]>" | ||
|
28689b5
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.
In my view, this should have been a semver major release due to the inclusion of #1017 which changed behavior, breaking the build/deploy toolchain at PayPal. We've worked around it this AM by pinning to the previous working version.
28689b5
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.
@aheckmann That would have been slightly problematic, considering that according to semver forever is still in beta. Could you please elaborate on particular use-case that broke down? Previous behaviour was more likely to obscure real issues, so if you are saying sometimes old behaviour is preferable, I need to understand what is the better default.
28689b5
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.
Yes, semver
0.x
is officially anything goes territory. However, IMO, when you have significant user base,0.
should be treated as1.x
to avoid large impacts.I agree previous behavior was obscuring real issues and am +100 on this fix (we were unaware we suffered from this issue, so thank you for the release). I'm instead recommending that because
forever
has significant quantity of users, it's time for1.x
.28689b5
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.
@aheckmann Great point! Yes, this is something I definitely wanted to do; but before that want to complete major cleanup...
28689b5
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.
You are right @aheckmann – will work with
npm
folks to get that version pulled. Overlooked the way that this PR does indeed change behavior in a breaking way – apologies.28689b5
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.
@indexzero Don't think change itself should be reverted, though. Just makes sense to re-release version as 1.0.0 and follow semver from now on.
28689b5
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.
Respectfully I disagree. I've emailed npm support folks to have
[email protected]
be unpublished from the registry. Once that is confirmed I will bump to1.0.0
and publish that version.28689b5
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.
@indexzero Not saying that version shouldn't be pulled :). Just saying that rereleasing same version without any changes as 1.0.0 makes more sense than reverting change and publishing as 0.15.5.
28689b5
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.
(and since that seems to have been your plan to begin with, all good!)
28689b5
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.
@aheckmann
0.15.4
has been pulled from thenpm
Registry.@kibertoad
1.0.0
has been published. Any help cleaning up theCHANGELOG.md
for the release or for any interim versions would be appreciated.28689b5
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.
@indexzero Sure, will do! By cleaning up you mean filling in the gaps for changes not currently listed there, or something different?
28689b5
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.
You got it – filling in the gaps 😸
28689b5
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 @indexzero @kbackowski !
28689b5
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.
@indexzero Done!
28689b5
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.
@indexzero can I ask why did you ask for unpublishing? We now have a trove of lockfiles to update.
28689b5
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.
@flovilmart Because it was a potentially breaking change, hence it was re-released as 1.0.0 instead. You can switch to that if you are ok with https://github.com/foreverjs/forever/pull/1017/files
28689b5
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.
welp, if I may suggest, next time, you can perhaps deprecate the package. There is a long history of people losing their minds over unpublished packages or npm refusing to unpublish. (npm/npm#16178).
I'd rather have a broken sofware than broken lockfiles.
28689b5
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.
@flovilmart Package wasn't unpublished, version was. Idea behind unpublishing specific version was to avoid CI breaking due to deployments automatically picking up versions in-range.
28689b5
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.
@kibertoad is correct @flovilmart – we missed the unpubilsh window by about 48 hours. The calculation of the future impact being much greater compared to the impact in that 72 hour period was why we made that decision.
28689b5
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.
potatoes / potatoes
And guess what..... my CI is broken because the package's version was unpublished... One way or the other, this leads chaos.
@indexzero I'm not saying it's easy, but in this case, there are other solution than unpublishing.
I hate that NPM has this option it is poorly used by everyone and as maintainers we should AVOID it altogether.
I don't want to spark any debate, but the poor guys at PayPal that trust pre 1.x sem-ver are equally to blame.