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

Add Node 12 support #187

Merged
merged 31 commits into from
Jul 16, 2019
Merged

Conversation

DavidVujic
Copy link
Collaborator

@DavidVujic DavidVujic commented Jul 13, 2019

Description

This PR will make it possible to build this project in Node.js 12. Both deprecated and removed V8 features have been replaced with new equivalent features. Deprecated Nan features have also been replaced. Besides being able to build in Node.js 12, all build warnings about deprecated functions are now fixed.

Motivation and Context

Node.js 12 will probably very soon be the next LTS. The changes in this PR are critical to make it possible for Node.js 12 projects using this library.

Fixes issue #153

How Has This Been Tested?

  • Built in Travis CI
  • unit tests
  • integration tests
  • manual testing, using the code in the examples folder
  • added new component tests: testing new C++ code (converters.h)

travis

npm test

total:     108
passing:   108
duration:  12ms

npm run integrationtest
(verbose output, skipped pasting here)

npm run test-components

total:     8
passing:   8
duration:  8ms

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

@DavidVujic
Copy link
Collaborator Author

I would very much appreciate to get feedback on this PR. My C++ skills are not good, and I am not sure about the new code written follow guidelines or handles memory issues correctly.

Maybe @kuebk can have a look?

Also @jbienkowski311 ?

src/converters.h Outdated Show resolved Hide resolved
src/node-zk.cpp Outdated Show resolved Hide resolved
src/converters.h Outdated Show resolved Hide resolved
Copy link
Contributor

@jbienkowski311 jbienkowski311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@DavidVujic DavidVujic merged commit 39e29f7 into yfinkelstein:master Jul 16, 2019
@DavidVujic DavidVujic deleted the node_12_support branch July 16, 2019 18:20
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

2 participants