Remove node version check

[adrian-castravete]

Fix an error where the version check of node is only applied to the minor number.
Since node is now at 4.1.0 the check for the minor version being greated than 8 fails.

[ghost]

This is just patching the wrong proprietary solution. The right way would probably be to use https://github.com/npm/node-semver and ask it to check if it passes >=0.8.0 (or, actually, rather >=0.10.0, as amber is not supporting 0.8 any more.

And maybe, even better, just package.json of amber-cli should contain proper engines (like >=0.10.0 <1.0.0 || >=4.0.0 and the code that checks the version should be removed altogether.

[adrian-castravete]

Oh so it's a little more complicated than expected... :( I'm not really that knowledgeable in versioning solutions... Until then it may be uninstallable on newer nodes... :/

[ghost]

Maybe it only looks "complicated" because I proposed the long defensive version string - to allow old node, skip intermediate io.js stage (just for the case, so we do not need to support strange edge cases) and allow the new merged node. Otherwise, plain >=0.10.0 would suffice.

Back then, amber command did not have its own npm package, as it has now, so the version checking code is probably from that old times.

I would say the version check code should be simply removed, and left to npm "engines" field in package.json, which s checked during installation (no need to check at runtime).

Adrian Castravete wrote:

Oh so it's a little more complicated than expected... :( I'm not
really that knowledgeable in versioning solutions... Until then it may
be uninstallable on newer nodes... :/

—
Reply to this email directly or view it on GitHub
#1201 (comment).

[adrian-castravete]

I agree with that! 👍 Let's remove version checking. 😸

[ghost]

Pls also change (or add, if not present) the engines in package.json, so that it only allows certain node versions. For example: 0.10.x || 0.12.x || 4.x

[adrian-castravete]

Hello @Herby ! I added the version limitations and removed the version check code.
It works. Running the npm install -g --engine-strict doesn't let me install amber on node 4.1.
I didn't put version 4 in there because of this issue.

[ghost]

I'll wait with merging until 4.x is there as well.

[adrian-castravete]

OK. I closed the issue. Here's the modified code. The versions now allow for anything between 0.10 and 0.13 (exclussively) and anything over 4.0. :)

It was awesome seeing it create the amber environment 😸

[ghost]

BTW you updated wrong package.json; the one you changed is for amber, not amber-cli. Never mind, I will updated that one.

[ghost]

Ah, sorry. You updated both. Taking it back.

[adrian-castravete]

No probs! 😆 Yeah, I saw they were both and updated. 😄
Glad I could help!

[ghost]

;-) If you want to help more, there are lots of issues out there. There are even ones labeled "entry level" for those who want to help but are not yet immersed enough.

[adrian-castravete]

Yes, I will look into it. Although it will mostly be in free time. 😃 Love the project and want to see it move forwards.

[ghost]

BTW if you happen to test that amber-cli now actually works in node 4, let me know, so I can make a new release of it.

[adrian-castravete]

Yeah, I tested it on an empty directory. It created everything without a problem. The only weird thing I noticed was some (probably CSS related) issues with displaying the rename protocol window in the Helios IDE. Otherwise everything works OK and everything gets saved as it should.

[ghost]

Helios was moved to bootstrap3 and there are still some visual issues. It is not related.

Thanks, going to release.

Adrian Castravete wrote:

Yeah, I tested it on an empty directory. It created everything without
a problem. The only weird thing I noticed was some (probably CSS
related) issues with displaying the rename protocol window in the
Helios IDE. Otherwise everything works OK and everything gets saved as
it should.

—
Reply to this email directly or view it on GitHub
#1201 (comment).