-
Notifications
You must be signed in to change notification settings - Fork 23
Updates Node releases from both release and rc download directories #43
Conversation
Builds on the work by Michał Gołębiowski to make two requests to both https://nodejs.org/download/release/ and https://nodejs.org/download/rc/ to add release candidates to the list of the Node versions and subsequently (as of 7th Sep 2015) show 4.0.0-rc.2 at /node/unstable - was previously incorrectly showing 0.12.7
@@ -33,12 +36,30 @@ NodeSource.prototype.update = function(done) { | |||
if (!res.text) return done(new Error('No response'), false); | |||
if (res.status !== 200) return done(new Error('Bad response'), false); | |||
|
|||
this._parse(res.text) | |||
done(undefined, true); | |||
responseText += res.text; |
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.
The indentation seems wrong here; compare to previous code (each line should be indented additional 2 spaces, here it's 4).
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.
Fixed
My comment disappeared due to an outdated diff so I'll repost here: There's a lot of duplicated code in this PR. Why not download from both URLs, concatenate bodies (maybe separating them with a new line) and have parseResponse check both responses? Also, note that /download/rc/ is a location of RCs but in the future there will also be betas/alphas, it should be decided what to do with them. For example, supporting nightlies would most likely be too much as they will arrive extremely frequently. (NOTE: I'm not a maintainer of this module so my comments may be overruled :)) |
I agree about the duplication but I wanted to keep this PR as simple as possible and not introduce any async/promise libraries. I'm more than happy to refactor this if need be. It's not entirely clear, to me, what the future release process will look like regarding alpha/beta/rc. I tried to decipher something from the following threads but they are fairly lengthy: https://github.com/nodejs/dev-policy#release-process-and-branching-model My main driver for this PR is that semver.io dictates which version of Node are avilable on Heroku and I believe there might be a significant desire to test out 4.0 especially as it will form the first LTS release with support until April 2017. |
One nit: 4.0 will not be an LTS, some future 4.x release will. The 4.x line will be normal Node stable line, it will turn into an LTS after some time. For more, see https://github.com/nodejs/LTS. |
I'm going to close this as the final release of 4.0.0 came out pretty much straight away after the release candidates and some of the recent commits to this repo handle the changes to Node distribution. It's likely we'll have to wait till October and Node 5.0.0 to get a clearer picture of how LTS and unstable releases of Node are going to be handled going forward. |
I'm happy to have the discussion up... now that 'stable/unstable' isn't such a clear cut thing in node versioning, we'll need to figure out what makes sense in terms of responses from semver. |
It's more that now there are more flavors of "unstable": nightlies, alphas, betas, rcs. All |
I've created an issue that can track discussion of this issue more broadly than this specific pull request. |
Builds on the work by Michał Gołębiowski to make two requests to both https://nodejs.org/download/release/ and https://nodejs.org/download/rc/ to add release candidates to the list of the Node versions and subsequently (as of 7th Sep 2015) show 4.0.0-rc.2 at /node/unstable - was previously incorrectly showing 0.12.7
I'm more than happy for any feedback on this and any changes that need to be made.