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

n-api: return napi_invalid_arg on napi_create_bigint_words #31312

Closed
wants to merge 1 commit into from

Conversation

@legendecas
Copy link
Member

legendecas commented Jan 11, 2020

N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

Refs: #29849

These changes shall not be made on existing N-API. But BigInt related N-API is still an experimental API.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.
@legendecas legendecas force-pushed the legendecas:napi_bigint_throwing branch from 326610c to ccc6cd5 Jan 11, 2020
@legendecas legendecas added the n-api label Jan 11, 2020
@legendecas legendecas marked this pull request as ready for review Jan 11, 2020
@nodejs-github-bot

This comment has been minimized.

@nodejs-github-bot

This comment has been minimized.

@Trott
Trott approved these changes Jan 13, 2020
Copy link
Member

mhdawson left a comment

LGTM since this API is still experimental and we'd discussed previously that we should be returning errors in this way versus throwing an exception.

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Jan 13, 2020

@nodejs-github-bot

This comment has been minimized.

Trott added a commit that referenced this pull request Jan 14, 2020
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

PR-URL: #31312
Refs: #29849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@Trott

This comment has been minimized.

Copy link
Member

Trott commented Jan 14, 2020

Landed in 689ab46

@Trott Trott closed this Jan 14, 2020
MylesBorins added a commit that referenced this pull request Jan 16, 2020
N-API statuses shall be preferred over throwing JavaScript Errors on
checks occurred in N-APIs.

PR-URL: #31312
Refs: #29849
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
@codebytere codebytere mentioned this pull request Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.