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

NaN ’NewInstance’ API fix #326

Merged
merged 1 commit into from Oct 10, 2018

Conversation

Projects
None yet
2 participants
@deszip

deszip commented Oct 9, 2018

NewInstance method in NaN seems to lack version with only two arguments (argc and argv). Maybe it was removed at some time. Only three versions exist for now: Source
I've changed js wrapper to use correct version.

@vixentael vixentael added the jsthemis label Oct 9, 2018

@vixentael vixentael requested review from Lagovas and vixentael Oct 9, 2018

@vixentael

This comment has been minimized.

Member

vixentael commented Oct 9, 2018

thank you @deszip!

so this should fix jsthemis for Node v10, right?

@deszip

This comment has been minimized.

deszip commented Oct 9, 2018

Yep, tested on node 10.9.0

@vixentael vixentael changed the base branch from master to vixentael/jsthemis-fix Oct 10, 2018

@vixentael

This comment has been minimized.

Member

vixentael commented Oct 10, 2018

Great, this PR will fix #325

  1. Will merge to vixentael/jsthemis-fix branch first to test with Nodejs < 10.
  2. Will merge to master next

@vixentael vixentael merged commit d2e6f53 into cossacklabs:vixentael/jsthemis-fix Oct 10, 2018

3 of 6 checks passed

ci/circleci: php5 Your tests failed on CircleCI
Details
ci/circleci: php70 Your tests failed on CircleCI
Details
ci/circleci: php71 Your tests failed on CircleCI
Details
ci/circleci: android Your tests passed on CircleCI!
Details
ci/circleci: integration_tests Your tests passed on CircleCI!
Details
ci/circleci: x86_64 Your tests passed on CircleCI!
Details
@vixentael

This comment has been minimized.

Member

vixentael commented Oct 10, 2018

I've made new PR #327 based on these changes.

@deszip take a look!

Updated version of jsthemis@0.10.1 is available in npm.

vixentael added a commit that referenced this pull request Oct 10, 2018

jsthemis update (support new versions of NaN and Nodejs) (#327)
* NaN ’NewInstance’ API fix (#326)

* update examples, update jsthemis.package json
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment