Fix syntax errors in Node 4

[DuBistKomisch]

The recent https://github.com/html-next/ember-hammertime/pull/40 used some syntax which is too new for Node 4.

[DuBistKomisch]

Not sure why the 2.12 test failed... works locally, but I can't retry the job on travis because I don't have write access apparently.

[RobbieTheWagner]

@DuBistKomisch can you not use node 6+? Node 4 is quite old.

[DuBistKomisch]

Sure I can, but maybe someone else can't. 4 is still supported as an LTS, and is supported by ember and every one of the dozens of other dependencies I have, why can't this addon?

[RobbieTheWagner]

@DuBistKomisch it could certainly support 4, but I think there is really no need to, at this point. I'm generally in favor of only supporting a couple versions back, and not all the way back to the earliest LTS. That being said, it doesn't hurt anything, other than code readability, to support 4, so if you want to rebase against master, it should fix the test failures.

[RobbieTheWagner]

@eriktrom I think let and const should work in node 4.

[eriktrom]

Yeah.

-- Erik

On Oct 14, 2017, at 1:12 PM, Robert Wagner notifications@github.com wrote:

@eriktrom I think let and const should work in node 4.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[eriktrom]

What precisely is this pr really trying to fix for node @dubistkomisch?

[RobbieTheWagner]

@eriktrom think it's destructuring is not supported in node 4. I'm still in favor of just dropping node 4 support, as it's going to die here before too long.

[eriktrom]

Agreed

[DuBistKomisch]

@eriktrom Specifically destructuring and let/const in a "global" scope is unsupported, they do indeed work otherwise. Trying to run ember build on Node 4.8.4 currently spits out cryptic syntax errors from internal addon loading code unless I make these fixes inside my node_modules.

https://emberjs.com/blog/2016/09/07/ember-node-lts-support.html

Ember CLI can be expected to work just the same running on a "Maintenance" Node.js version as it does on a "Current" Node.js version.

https://github.com/nodejs/Release

Node 4 is "Maintenance" for at least another 6 months, it's only half way through. This is an Ember addon, shouldn't it try to follow Ember's Node support policy, rather than arbitrarily drop versions? The majority of the code can be transpiled by babel anyway, it's just stuff parsed directly by node (i.e. index.js) that needs to work in Node 4. I don't see why it should be dropped now, it's just a few syntax restrictions.

[RobbieTheWagner]

@DuBistKomisch I just don't agree with supporting 5 major versions back of node, since 9 is almost out. While ember does support 4, I think it really restricts us to support so far back, and we should push forward on ES2015+ and modern versions of node. Node 4 is practically the same as node 0.12, and it really should be discouraged heavily.

[DuBistKomisch]

It's not 5 major versions, it's 2 stable versions (8 isn't even LTS yet), and anyone who tries to use 9 for production is insane. Version numbering is time and stability based in Node.

This stuff has already been decided, you should wait to drop Node 4 support until when the support window ends in April, it's not your place to "discourage" its use by breaking builds in a patch (!) version.

[RobbieTheWagner]

@DuBistKomisch LTS just means they are going to support it for awhile. In node, an LTS happens every other major version (6, 8, 10) etc. Just because 5 and 7 did not have an LTS does not mean they were not stable or usable. Therefore, with 9 coming out very soon, 4 is 5 major versions ago, and very outdated.

Node 4 came out 2 years ago, and while it does receive critical bug fixes, it lacks a ton of features.

I've been constantly updating versions and I've had no issues. There really is no downside to grabbing a newer version of node. I've never had anything break because my node version was too new.

I'm okay with merging this, but I don't think you should expect people to support things that are a couple years old. Everything changes constantly in JS and it's always best to try to stay on the latest standards you can.

[DuBistKomisch]

I've updated a complex node project from 0.10 to 0.12 to 4 and many parts broke and required rewriting between each, so I'm not as lucky as you I guess. ember-cli itself was broken on Node 8 when it first came if you want an example of too new. Being on the bleeding edge isn't a realistic expectation for everyone either, it's best to support as much as possible both ways.

I don't expect "people" to support Node, I literally fixed the code myself and gave it to you, I don't see what the big deal is here.

[RobbieTheWagner]

@DuBistKomisch things are not going to break between node versions in ember-cli often. Definitely curious what problems you are referring to.

[DuBistKomisch]

Off the top of my head, the biggest was that https://github.com/nodejs/nan got a new v2 API which was the only one supported in Node 4 (and this is happening again soon with the new n-api in Node itself, that should be fun), so I had to rewrite all the glue in our C++ addons which we use heavily.

For 0.10 to 0.12 there were changes like how IP addresses can be looked up from socket connections, and the new streams API broke some of the ways we did networking (which I never fixed, luckily the new version of what we were interfacing came out and I had to rewrite for that anyway).

There was also a massive memory leak in 0.12 to do with using custom SSL certs which is what forced the update to 4, and using custom SSL certs was necessary because Node ignores system certs until around 7.5. It was backported to around 6.10 so there's one thing that needs rewriting when I update soon.

\</rant>

edit: Also ember-cordova or probably just cordova itself was totally broken with no error messages on Node 5, which is when I started locking our ember stuff to 4 as well.

[RobbieTheWagner]

@DuBistKomisch I don't think any of those things really apply to 99% of Ember addons. Would certainly be a problem writing a node app though. I'd highly recommend trying to update and see if things work for you now. If there are addons that are broken on node 6 or 8, I am happy to help fix them. I believe strongly we should get people on the newest everything we can.

[DuBistKomisch]

Sure, but the 1% can be enough to hold you back, regardless of the rest. Obviously keeping up to date is important but I think supporting LTSes, especially when ember does is, is a very reasonable idea, especially when all it takes is a 5 line diff.

[RobbieTheWagner]

@DuBistKomisch sure, but definitely tell me any addons broken on 6 or 8 and I'll fix them. The reverse is also true. Just because there is an LTS doesn't mean we shouldn't support all the newest versions.

[DuBistKomisch]

I'll report any I find, but this addon was the only one breaking stuff recently and #40 (mostly) fixed that so thanks already. 😛

[eriktrom]

so I had to rewrite all the glue in our C++ addons which we use heavily.

um so yeah this is very rare, but i totally understand now. There are new api's coming out, if ur using N-API(soon) or C++ addons - within ember-cli's build... there may be some issues b/c this document seems to indicate that all new pushes to master for ember-cli itself, are no longer being tested with node 4 - https://github.com/ember-cli/ember-cli/blob/master/docs/node-support.md#ongoing-support-per-this-rfc

that said - your living on the edge - if u come across other addons that need node 4 support until u upgrade N-API - your request sounds reasonable. Although in ember-cli itself, you may want to consider not upgrading per the chart linked above (or raise this with the core team, fyi)

thanks for the clear explanation @DuBistKomisch - honestly, i didn't get what u needed, till now :)

[DuBistKomisch]

@eriktrom thanks, I haven't seen that document before but it's a little confusing. It was published after april 2017 but refers to it in the future for dropping node 4... have to assume it's a typo and they mean april 2018 as expressed in the blog post I linked earlier. Their travis config still runs 4 as well.

I'm waiting to update to Node 6 or 8 (which shouldn't require rewriting to n-api afaik) at the same time as our suppliers give us a debian stretch image (since I may as well break everything once instead of twice) but they're taking a while, so you can see I'm kind of stuck for now.

[eriktrom]

word - if u run into other addons, as @rwwagner90 offered as well, feel free to ping me, given i understand your circumstance (which turn out to be super legit, albeit rare for shiz)

[RobbieTheWagner]

To be clear, I was offering to help fix addons broken on node 6 or higher (of which I would assume to be almost none), I still support killing 4 ASAP 😃