NodeJS v4 support - fixing the failed build

[lirantal]

Adding the required support to properly build the nodejs v4 edition.

Source and credit originated from: lirantal#1

[ilanbiala]

@lirantal 4.0 still failed. Check Travis. Also, if we fix Node 4.0 support, then we should remove the allow_failures rule for it.

[lirantal]

yep, still more work to do there

[lirantal]

there's an open issue (socketio/socket.io#2228) with socket.io ability to build on node v4 now, I've been trying some magic but so far no luck. Another option for us is to add shrinkwrap (which I think is a good idea so I'll take a look at it) to control our own sub-dependency versions if we need to.

[mleanos]

👍 On skrinkwrap. I was looking at that a while ago, and thought it was a good solution for this project.

[vielmetti]

[email protected] is said to have node.js 4.x support. https://twitter.com/SocketIO/status/645755967934697472

[vielmetti]

I was also having issues with one of the dev dependencies, grunt-node-inspector, which is at 0.2.0.

[lirantal]

yep, saw that and also Edward from the original PR gave me a ping about it.
I'll update the PR soon enough.

[ilanbiala]

If we are attempting to get Node 4 passing, we should remove this section.

[lirantal]

I don't think that's the reason for it, but rather node-inspector which is failing.

[ilanbiala]

@lirantal any progress on this?

[lirantal]

I pushed an update to a newer grunt-node-inspector version which seem to be the culprit, let's see how it goes.

[ilanbiala]

@lirantal 1 test failed. Look at the bottom https://travis-ci.org/meanjs/mean/jobs/83342174

Also, if we are going to make sure Node 4 builds and works, please remove the allow failure part for it because it shouldn't be there anymore.

[lirantal]

Yeah I just noticed that it finished with a fail for a test, which is related apparently to different messages between node/other package versions:

1) User CRUD tests should be able to login successfully and logout successfully:
      Uncaught AssertionError: expected 'Found. Redirecting to /' to be 'Moved Temporarily. Redirecting to /'
      + expected - actual
      +"Moved Temporarily. Redirecting to /"
      -"Found. Redirecting to /"

I'm not sure if we want to rely 100% now on node v4 so I say let's keep the allow_failures there still.. As you saw there are many core packages like grunt-node-inspector and others that didn't yet even sync with the node v4 version to become compatible.

[codydaig]

@lirantal LGTM. I say merge this in, to get people closer to Node 4.0 support. It's been a popular issue lately.

[ilanbiala]

@lirantal I'd rather not merge and keep allow failures, it is deceiving.

[lirantal]

@ilanbiala what's deceiving? I don't get it. It's not like the project states it is compatible with v4.
We always had allow_failures on for v4 and I think it should be kept this way for a while until the ecosystem becomes more aware and compatible with v4.

[ilanbiala]

@lirantal most people don't know about allow_failures, and they won't look closely enough to notice that v4 doesn't have to pass for the badge to show passing.

[lirantal]

That's why we as moderators should look into that.
You suggest that remove allow_failures and let the build fail entirely if some PR doesn't meet v4 compatibility but that potentially has the possibility of delaying many PRs from being accepted because all the PRs will always fail, and the fix for node v4 might be out of our reach and not so quickly available.

I don't see the point in failing the build entirely for v4 compatibility for the time being.

[codydaig]

I don't think it should fail on node v4 until we start to officially support it.

[lirantal]

That's what I'm saying... it currently fails on some test as well that we need to fix to make the build pass v4.

[bmcorser]

I was getting a bunch of explosions with npm install for this repo, but I can to run without erroring if I run npm install grunt-node-inspector beforehand as in node-inspector/v8-debug#7 (comment). That issue suggests that https://github.com/ChrisWren/grunt-node-inspector/ references an out of date node-inspector package ... is that what this PR sets out to resolve?

[bmcorser]

Hmm seems like v4 isn't quite supported and I should go back to v0.12 ...

[lirantal]

@bmcorser indeed.

[bmcorser]

@lirantal 👍 ty

[lirantal]

  1) User CRUD tests should be able to login successfully and logout successfully:
      Uncaught AssertionError: expected 'Found. Redirecting to /' to be 'Moved Temporarily. Redirecting to /'
      + expected - actual
      +"Moved Temporarily. Redirecting to /"
      -"Found. Redirecting to /"

The specific test that fails here is because NodeJS v4 changed their status codes for 302 from "Moved Temporarily" to "Found".

I was thinking to support both v4 and prior versions we can check for either of the strings, so I'll update the tests unless you have another idea.

[lirantal]

Since the problem here is the actual nodejs version how about we have a case for each version in the test? i.e:

if (process.version.indexOf('v4'))
  signoutRes.text.should.equal('Found. Redirecting to /');
else // it's otherwise v0.12, or v0.11 then
  signoutRes.text.should.equal('Moved Temporarily. Redirecting to /');

WDYT?

[ilanbiala]

@lirantal is it "v4" or "4."? I guess we can do that.

[lirantal]

Cool, I'll make the changes and commit so we can see the CI tests pass.

[lirantal]

@ilanbiala node v4 is passing just fine now.

[ilanbiala]

Why did this get removed?

[lirantal]

@ilanbiala I removed it in the beginning when working on this PR for node v4 compatibility
I re-added it again now.

[codydaig]

@lirantal It is failing on the e2e tests, but grunt is continuing and not producing the error too fail the build. Not sure if that is related or not.

[ilanbiala]

Here is where things go awry: https://travis-ci.org/meanjs/mean/jobs/83604705#L1555

[lirantal]

The E2E test fails for both v0.10 and v0.12, it's not related to node v4.
I opened a separate issue to deal with it: #961

This PR LGTM to continue with.

[ilanbiala]

LGTM, squash and merge!

[codydaig]

LGTM

Thanks @lirantal!

[lirantal]

Squashed and merged.
Thanks guys!