Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update node version to v 6.1.0 #230

Merged
merged 1 commit into from
Mar 28, 2017
Merged

Conversation

geowarin
Copy link
Contributor

Hello and thanks for J2V8.

On my mac, I noticed that upgrading node to v6.1.0 did not cause any compile error generating the dylib.

Node 6.0.0 has several advantages:

  • 99% ES 2015 compatibility through v8 v5.0.71.35
  • It makes ordering of the Object.assign() keys compliant with the spec. It seems boring but it actually solves a very nasty bug with React Server-side rendering.

I just wanted to begin this discussion with you and know why node version hasn't been updated recently.

v6.1.0 is the upper major version which compiles without errors when linking the binary (6.2.0 fails with errors beyond my skill level).
We could probably pinpoint the exact commit that introduces changes in C++ API with git bisect if we choose to keep updating the lib.

I realize this is a big effort in testing and I'm also interested in any guidance you can provide regarding testing, QA and CI.
I would be more than happy to contribute to the documentation as well.

@devsnek
Copy link

devsnek commented Feb 25, 2017

it might be worth looking into working with the new node v7 generateEnvironment changes (it doesn't need a uvloop spawn anymore) and working to integrate with that, as 7.6.0 moves to v8 5.5 which has several advantages. like async/await and memory improvements (yay!)

@tnytown
Copy link
Contributor

tnytown commented Mar 14, 2017

I've actually forked and gotten J2V8 to work with nodejs/node@4760abc (v7.4.0) a while back. Took some tweaking (you can see the changes at KnownUnown/J2V8@a0af163), but it works, on Android at least. There might be some unknown issues / memory leaks / surprise pterodactyl attacks - I'm a C++ novice. Probably will pull request back to this repo soon.

@winston01
Copy link

winston01 commented Mar 17, 2017

@knownunown : using your fork I managed to build it on Linux too, although there are a few Failures and Errors in maven tests that should be checked.

@irbull
Copy link
Member

irbull commented Mar 28, 2017

Moved this to the J2V8 repo under the branch https://github.com/eclipsesource/J2V8/tree/node-v6.1.0. I'm looking at this now on my machine. Thanks so much @geowarin and others.

@devsnek
Copy link

devsnek commented Mar 28, 2017

i think it would be preferable to move directly to v7, or even better v8 since v7 is already release and v8 will be release soon

@irbull
Copy link
Member

irbull commented Mar 28, 2017

@GusCaplan I agree. I just want to catch up on this discussion. Also, because we have the intermediate of version 6, I want to try that first (easier to move in baby steps if there are API changes).

@irbull irbull merged commit 7dedc23 into eclipsesource:master Mar 28, 2017
@irbull
Copy link
Member

irbull commented Mar 28, 2017

This works, and I think we should put this one in. We can move directly to version 7 or 8 in the builds, but having this commit in the repo won't hurt (especially if someone needs to checkout and build version6). Thanks everyone.

The only comment I have is the test that checks versionStartWith4. While the test has been updated, the test method name hasn't. However, because github makes it hard for others to collaborate on pull requests, it's easier to accept this and make the change in a subsequent commit. I'll do that.

Thanks, everyone, and now I'll start working on version 7.

@irbull
Copy link
Member

irbull commented Mar 29, 2017

I've grabbed @knownunown's fork, but I can get it to build on my mac. no type named 'unique_ptr' in namespace 'std', this is in some v8 header file. If anyone has any pointers, please let me know, otherwise I'll dive in again tomorrow.

Thanks again everyone!

@devsnek
Copy link

devsnek commented Mar 29, 2017

@irbull i found the same thing earlier and mentioned him in #237

@muthu90ec
Copy link
Contributor

Quick question, when could i expect the new j2v8 with node v6+ available in maven central repo? Thanks..

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants