Fix for Node LTS compatibility

IgnoredAmbience commented 5 years ago

Fix Compare.js in the generator testsuite to work with Node <9
Travis CI should now use lts/carbon (8.x) for testing.

Fixes #29

IgnoredAmbience commented 5 years ago

@barockobamo This should fix your issue, if you could test?

barockobamo commented 5 years ago

I have just upgrade to Node v10 :) , i will test somewhere and give you the feedback.

IgnoredAmbience commented 5 years ago

Oh, the point was that we should be supporting v8, so testing on that platform would have been good. I've now managed to get the CI to use v8 of node to test it...

barockobamo commented 5 years ago

I am having many issues testing this on my debian VM with opam v1.2.2 and node v8.12.0. In the node_modules directory i get only esprima. I will check again with node v10 to be sure that it behave normally.
I suggest that the npm and nodejs install should be done locally in the project folder during the build and not as a system dependency because this will cause many configuration failure for the users as i am experimenting it at this point.
Managing it as a local install in the build folder will give us the control of the nodejs version and would forbid the user to decide for us. And we would not impact or confict with any previous nodejs system installation.

IgnoredAmbience commented 5 years ago

I remain of the opinion that we should not be bundling nodejs/npm with this repository. We should clearly state the version requirement (8+), as this is the LTS release which should be provided by most LTS linux distributions. Debian has it in stretch-backports, Ubuntu in 18.04 (16.04 is now out of support), it present in the supported releases for Fedora, but not CentOS.

In the event that the OS does not have version 8+ packaged, we should recommend that a more recent version is obtained using nvm. https://github.com/creationix/nvm

I've also just noticed that the dependencies aren't listed in the toplevel readme. I'm sure I had added them...

barockobamo commented 5 years ago

Ok, the problem was that i was running make test without make test_init before. So node can stay as an external dependence. But my test on debian with node v8.12.0 and opam 1.2.2 is KO anyway : here is how it fail :

Wrote file: displayed_sources.js make[1]: Leaving directory '/home/test/jsexplain/jsref' node_modules/.bin/mocha 1) "before all" hook 0 passing (493ms) 1 failing 1) "before all" hook: Uncaught TypeError: Cannot read property 'doesNotThrow' of undefined at fs.readFile (test/interpreter.js:34:12) at FSReqWrap.readFileAfterClose [as oncomplete] (fs.js:511:3) Makefile:35: recipe for target 'test_jsjsref' failed make: *** [test_jsjsref] Error 1

IgnoredAmbience commented 5 years ago

Thanks for that bugreport. I'll take another look into it. Looks like the CI build wasn't testing the generator on each build, have now fixed that too.

IgnoredAmbience commented 5 years ago

It looks like this is now fixed.

barockobamo commented 5 years ago

Just finish the test, it's OK, (my VM is very slow, i 'am making a new one)

IgnoredAmbience commented 5 years ago

Yes, the test262 suite takes a long time to complete. It's not very suitable to run to completion for every change made.

jscert / jsexplain

Fix for Node LTS compatibility #31