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

feat: Compatibility for Node 16 #41

Merged
merged 3 commits into from
Oct 15, 2021
Merged

Conversation

Hazmi35
Copy link
Contributor

@Hazmi35 Hazmi35 commented Aug 10, 2021

Nodejs 16 uses V8 engine version 9.0, that requires c++14 features, so we need to remove "-std=c++11" from binding.gyp.
Idk why "-std=c++11" is added in f3c64a0

REFS:
nodejs/node-gyp#2387 (comment)
nodejs/node#38367

@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Aug 10, 2021

Ah, my bad. I didn't see #40, should I close this PR?

@ExperiBass
Copy link

I'll just merge with mines if I can

@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Aug 12, 2021

I'll just merge with mines if I can

Okay

@Hazmi35 Hazmi35 closed this Aug 12, 2021
@Hazmi35 Hazmi35 reopened this Aug 13, 2021
@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Aug 13, 2021

Reopening this PR because #40 is closed

@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Aug 15, 2021

I forgot to delete .node-version file. I used it for fnm to test with Node v16.

@jhgg
Copy link
Member

jhgg commented Aug 15, 2021

Can you delete the package lock?

@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Aug 16, 2021

Can you delete the package lock?

Hi sorry, for the late response, I just deleted the lockfile @jhgg

@Zoddo
Copy link

Zoddo commented Aug 16, 2021

While you are at it, merging #42 would be useful too :)

@jhgg
Copy link
Member

jhgg commented Aug 16, 2021

@adill - when you get a chance, can you double check that this looks sensible?

@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Aug 16, 2021

Edited the first message for an explanation. Note that this has not been tested in Mac OS, but it works on Node.js v8 to v16 on Arch Linux, Node.js v8 to v16 on Windows (build 19044.1165, msvs 2019), and Node.js v14-v16 on Alpine Linux (docker)

@fredkilbourn
Copy link

@jhgg @adill Hi guys, its been a week just bumping to see if you have a minute to review/merge this yet. Thanks!

@Milo123459
Copy link

Yea, eta would be good. Other pr is fucked.

@satoufuyuki
Copy link

any progress?

@fredkilbourn
Copy link

@jhgg @adill weekly bump, apologies

@Hazmi35
Copy link
Contributor Author

Hazmi35 commented Sep 23, 2021

Hi, sorry for the bump, any progress on PR Review? Just a friendly reminder that Node 16 will be an Active LTS next month.

mzrtamp pushed a commit to stegripe/rawon that referenced this pull request Sep 24, 2021
Still no ETA until hzmi's fork merged, use hzmifork or don't add this [discord/erlpack#41](discord/erlpack#41)

Co-authored-by: Zen <45705890+KurokuTetsuya@users.noreply.github.com>
chakany added a commit to chakany/erlpack that referenced this pull request Sep 24, 2021
@fredkilbourn
Copy link

Sorry again, but bump

@fredkilbourn
Copy link

Another 2 weeks bump, is it possible to get this merged? @jhgg @adill

@msciotti
Copy link
Contributor

Got final signoff from Jake and Andy. Thanks for this!

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.

None yet

8 participants