Conversation
|
@devsnek. this would also need tests and doc to land. |
|
@mhdawson working on it locally, mostly just uploaded now to get comments on the design |
|
how should one add features that are under NAPI_EXPERIMENTAL in node_api? should we use the same preprocessor variable in node-addon-api? |
|
@devsnek correct, our plans is to use the same NAPI_EXPERIMENTAL define in node-addon-api for any method that depends on an experimental feature. |
devsnek
force-pushed
the
feature/bigint
branch
5 times, most recently
from
baa8b4f to
dd50036
Compare
devsnek
force-pushed
the
feature/bigint
branch
3 times, most recently
from
ce7c06d to
aea9def
Compare
NickNaso
left a comment
NickNaso
left a comment
There was a problem hiding this comment.
Great work. Just some notes about documentation
doc/bigint.md
Outdated
| static BigInt New(Napi::Env env, uint64_t value); | ||
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` Environment |
There was a problem hiding this comment.
The input parameter is Napi::Env and in the rest of documentation we defined this parameter as "The environment in which to ..."
doc/bigint.md
Outdated
| const uint64_t* words); | ||
| ``` | ||
|
|
||
| - `[in] env`: The `napi_env` Environment |
There was a problem hiding this comment.
The parameter env is Napi::Env and in the rest of documentation we defined this parameter as "The environment in which to ..."
|
@NickNaso comments were addressed |
|
anything else that needs to happen here? |
|
I'd like to find time to take a closer unless someone beats me to landing it. |
|
ping @mhdawson |
|
Sorry still recovering from being out last week, once I catch up will try to get back to this. |
|
ping @nodejs/addon-api |
|
@nodejs/n-api Can this be merged now? |
mhdawson
left a comment
mhdawson
left a comment
There was a problem hiding this comment.
LGTM after requested doc update
devsnek
force-pushed
the
feature/bigint
branch
from
e3cc237 to
e44aca9
Compare
|
This seems to have broken the CI see: https://ci.nodejs.org/job/node-test-node-addon-api/MACHINE=ubuntu1604-64/708/console |
|
@devsnek FYI |
Since original submit for nodejs#292 warnings were tightened through nodejs#315 causing the bigint test to fail to compile Fix the compile failure
|
submitted #345 to fix compile failure. |
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
Since original submit for nodejs/node-addon-api#292 warnings were tightened through nodejs/node-addon-api#315 causing the bigint test to fail to compile Fix the compile failure PR-URL: nodejs/node-addon-api#345 Reviewed-By: : None, landed to unbreak CI
5 participants
blocked until nodejs/node#21226 lands