Final 6.x release before EOL? [Discussion]

[BethGriggs]

There are currently 8 commits on v6.x-staging, and 2 open backport PRs targetted at Node 6.

I think we should discuss whether we should do a final release including all or a subset of the commits/PRs.

[richardlau]

@ljharb also asked in IRC and #405 (comment) whether the instanceof Array fixes from https://github.com/nodejs/node/pull/20250/files#diff-286202fdbdd74ede6f5f5334b6176b5cL320 could be backported.

[richardlau]

These are the commits on v6.x-staging that are not in a release:
nodejs/node@bc916e9 tools,gyp: fix regex for version matching
nodejs/node@16661e6 build: fix configure script for double-digits
nodejs/node@33cd51e tools: update tooling to work with new macOS CLI …
nodejs/node@74d3f0b src: fix bootstrap_node on bsd
nodejs/node@3c4949a doc: document that addMembership must be called once in a cluster
nodejs/node@29748cb doc: simplify CODE_OF_CONDUCT.md
nodejs/node@97d8bc8 build: only check REPLACEME & DEP...X for releases
nodejs/node@e2faf8c test: fix test-repl-envvars

[richardlau]

My thoughts:

nodejs/node@bc916e9 tools,gyp: fix regex for version matching
nodejs/node@16661e6 build: fix configure script for double-digits
nodejs/node@33cd51e tools: update tooling to work with new macOS CLI …

These commits are backports relating to fixing building Node.js on macOS with Xcode 10 however Xcode 10 has also removed libstdc++ which Node.js 6 uses (Node.js 8 and above use libc++) which we can't switch from without risking ecosystem breakage for prebuilt addons: nodejs/node#24648. It's questionable whether these commits on their own would actually fix anything.

nodejs/node@74d3f0b src: fix bootstrap_node on bsd

Fixes a bad bsd-only backport nodejs/node@77a405b that landed in 6.14.2. We should include this if doing another release.

nodejs/node@3c4949a doc: document that addMembership must be called once in a cluster

¯\_(ツ)_/¯

nodejs/node@29748cb doc: simplify CODE_OF_CONDUCT.md

Unnecessary IMO (does anyone read this on non-master?). doc changes (this and the one above) are reasonably low risk so could be included if another release was done, but not worth doing a release for.

nodejs/node@97d8bc8 build: only check REPLACEME & DEP...X for releases
nodejs/node@e2faf8c test: fix test-repl-envvars

These are only necessary if we actually do another release.

[richardlau]

Of the two pull requests:
nodejs/node#22633 falls under the N-API process under review at #410 (currently no objections).

nodejs/node#25939 looks like it is related to the security fixes in the recent v6 releases so probably warrants a new release?

[richardlau]

There's also nodejs/node#25447 for AIX which fixes a regression introduced by nodejs/node#17604 which was backported to v6.14.2.

[BethGriggs]

@nodejs/releasers @nodejs/lts - are there any concerns about doing this release with the list (or subset) of commits. I should be able to find time to prepare the release if there are any concerns about the labour involved.

[MylesBorins]

I'm +1 on a final release with the above changes.

[mhdawson]

@BethGriggs no concerns from me, but based on the comments it was not clear to me if all of the commits from the original list apply. Can you clarify the specific list that would be included?

[BethGriggs]

Draft commit list:

• nodejs/node@bc916e9 tools,gyp: fix regex for version matching
• nodejs/node@16661e6 build: fix configure script for double-digits
• nodejs/node@33cd51e tools: update tooling to work with new macOS CLI …
• nodejs/node@74d3f0b src: fix bootstrap_node on bsd
• nodejs/node@3c4949a doc: document that addMembership must be called once in a cluster
• nodejs/node@29748cb doc: simplify CODE_OF_CONDUCT.md
• nodejs/node@97d8bc8 build: only check REPLACEME & DEP...X for releases
• nodejs/node@e2faf8c test: fix test-repl-envvars
• [v6.x backport] src: add environment cleanup hooks
• [v6.x] http: fix error check in Execute()

And the the following if they land cleanly or a backport is raised

• http: refactor outgoing headers processing
• build: set -blibpath: for AIX

[BethGriggs]

I'm leaning against the macOS with xcode 10 fixes based on @richardlau's comments - I have not seen any requests for those, and if those commits are not guaranteed to fix anything then perhaps it is not worth the risk?

[richardlau]

And the the following if they land cleanly or a backport is raised

• build: set -blibpath: for AIX

nodejs/node#25447 doesn't land cleanly on v6.x-staging but the v8.x backport does. Raised nodejs/node#26478.

[BethGriggs]

nodejs/node#20250 requested by @ljharb doesn't land cleanly on v6.x so will need a backport. I'm a little concerned that the PR is referenced in some ecosystem breakages, do we want to risk breaking anyone on v6.x this late? Does it need to land with nodejs/node#24006?

Thoughts @nodejs/lts @nodejs/backporters?

//cc @apapirovski

[ljharb]

@BethGriggs the references, i believe, are that the PR fixes an issue - by backporting the Array.isArray changes in particular (not the rest) you’ll be allowing the ecosystem to remove its workaround for node 6+.

[MylesBorins]

@ljharb that is not the case. This change did indeed cause ecosystem breakage... I'll dig in a but more but my current gut is leaning to not making such a disruptive change at the end of LTS

[ljharb]

@MylesBorins i have no interest in the http header changes - only in the use of Array.isArray over instanceof Array - i can’t conceive of how that change alone could cause breakage. Could that alone be backported?

[MylesBorins]

Beth's initial assessment was spot-on. The change broke monkey patching the header response from 10.1.0 to 10.15.1 it also broke other things that required folks to make changes like antongolub/reqresnext@cf19b6d

If what Beth is saying, that the change requires manual rebasing with lots of changes I am unfortunately strongly -1 unless we can show that there is a significant regression fixed by this

While I appreciate the concern regarding the 6, specifically regarding removing a work around, the risk of larger ecosystem breakage has me concerned as we have only just fixed this in 10.x

Is there an issue that better explains the work around?

[ljharb]

jestjs/jest#7700 (comment) and jestjs/jest#5995 (comment) are the comments that led me to pursue backporting it - it's a bug in jest that seems unlikely to get fixed, which means jest is broken in some cases in node < 10.

Obviously we can still backport it to 8; it's just nice if we can also do that for 6, since node 6 will be around for awhile (just like node 4's still around, EOL or not).

[mhdawson]

@ljharb I'll agree with Myles that we need to balance risk versus benefit. If it's a nice to have but risky due to not landing cleanly, having not been validate for a while in earlier releases it may not make sense.

One question I'd have is if it's important why is it unlikely to be fixed in jest?

[ljharb]

It’s impossible or impractical to fix it in jest, i assume, since instanceof Array inside node has a reference to the original Array, and jest provides a different Array (and has no access necessarily to the original one) - whereas Array.isArray in node would work on anything that’s a subclass of a proper array, across realms as well.

[BridgeAR]

@ljharb I just had a look at this and I am confused as it seems already fixed in the latest v6? See https://github.com/nodejs/node/blob/v6.x/lib/_http_outgoing.js

[ljharb]

I’m a bit confused too. I will admit i read the jest issue and assumed that instanceof Array was being used in all node versions prior to that fix; if in fact the bug was added after node 6, then i apologize for all the added noise here.

[BridgeAR]

Closing as outdated. The last v6 release is already out.