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

Node 12 compatibility #53

Merged
merged 4 commits into from
Jun 9, 2019
Merged

Node 12 compatibility #53

merged 4 commits into from
Jun 9, 2019

Conversation

andreas
Copy link
Contributor

@andreas andreas commented Apr 29, 2019

With these changes, I can install the package locally using Node v12.0.0.

I'm no V8 expert, but used the following sources to come up with this patch:

@andreas
Copy link
Contributor Author

andreas commented Apr 29, 2019

It builds on Travis with Node v8, v10 and v12, but fails with older versions.

@holm
Copy link

holm commented Jun 5, 2019

@dachev any chance you could review this PR, as it is blocking us upgrading to Node 12

@dachev dachev merged commit 50b562c into dachev:master Jun 9, 2019
@dachev
Copy link
Owner

dachev commented Jun 9, 2019

@andreas @holm Thank you so much for the fix and I apologize for the long delay. Tested, merged and released!

@holm
Copy link

holm commented Jun 9, 2019

Excellent, thanks a lot.

@andreas andreas deleted the node12 branch June 9, 2019 18:54
@andreas
Copy link
Contributor Author

andreas commented Jun 9, 2019

Thanks @dachev!

@holm
Copy link

holm commented Jun 9, 2019

I was just testing this, and it doesn't seem like the right code got published. On node 12:

  • npm install cld@2.5.0 fails like before this PR
  • npm install cld@https://github.com/dachev/node-cld#4ef203d718e2603aeefd477a146a4c7a32bf32a8 works

@dachev
Copy link
Owner

dachev commented Jun 9, 2019

@holm Thanks for catching this. I pushed 2.5.1 and this time I confirmed that it actually installs.

@elhoyos elhoyos mentioned this pull request Jun 10, 2019
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

3 participants