adds experimental napi_date #497

mathiask88 · 2019-06-14T23:08:20Z

I tried to add the experimental date stuff. I'll add the docs if the rest is correct.

NickNaso

Hi @mathiask88,
good work, thanks for adding this new API. There are some tasks to address. Hope to hear you very soon.

napi.h

mathiask88 · 2019-06-16T13:54:26Z

I added some docs, but the current doc structure and naming is not fully consistent. I tried to stay file scope consistent.

NickNaso

Some little comment about the documentation.

doc/date.md

mhdawson · 2019-06-17T21:18:09Z

On other through, the doc sections should probably indicate that the Date related parts are experimental.

mathiask88 · 2019-06-17T21:29:02Z

@mhdawson True, I thought about it, but didn't add anything because BigInt isn't explicitly indicated as experimental. I'll add it and open another PR for all the mentioned doc stuff. Why was Date as a basic JavaScript object even added that late to N-API? And is it experimental because every new stuff starts as experimental? Just curious.

mhdawson · 2019-06-17T21:42:01Z

@mathiask88, new functions are added to N-API as experimental. This allows us to properly managed what is in each N-API version and confirm that the new functions don't have fundamental usage issues before we can no longer change them. N-API does not cover all of the JavaScript spec, instead we drove the APIs based on what we saw being used in the most used modules. So it's just most likely that Date was not in the set that we'd looked at.

As I wrote my comment I was thinking that we probably were not good about marking things as experimental before but I still think its a good idea going forward. Just to say that I understand why it would not have been evident that it was something to do.

mhdawson · 2019-06-17T21:43:14Z

@mathiask88, also wanted to say thanks for being so understanding. We still have some work to go in getting the module to be consistent and to iorn out how we handle experimental/new functions.

NickNaso

It's ok for me just one last little task. Good work @mathiask88 .

napi.h

mhdawson

LGTM

mathiask88 · 2019-06-28T15:25:19Z

@mhdawson Thanks, but there is still an open question #497 (comment)

mhdawson · 2019-06-28T18:41:34Z

@mathiask88 good point, I've commented above that I'm ok with valueOf instead.

NickNaso

LGTM

gabrielschulhof · 2019-06-28T19:10:59Z

Shouldn't it be ValueOf(), with a capital 'V'?

NickNaso · 2019-06-28T19:23:04Z

@gabrielschulhof yes you're right. @mathiask88 could you change the method name like repoter by @gabrielschulhof ?

mathiask88 · 2019-06-28T19:25:47Z

@gabrielschulhof @NickNaso Oh, sure. I was in JavaScript lower camel case land :)

gabrielschulhof · 2019-06-28T19:28:21Z

@gabrielschulhof @NickNaso Oh, sure. I was in JavaScript lower camel case land :)

😂 I know the feeling.

NickNaso · 2019-07-17T17:02:35Z

Hi everyone,
is it possible merge this PR?

gabrielschulhof · 2019-07-17T19:18:28Z

@NickNaso I'll start the CI.

gabrielschulhof · 2019-07-17T19:24:21Z

CI:

Version	Job	Status
13	https://ci.nodejs.org/job/node-test-node-addon-api-new/245/
12	https://ci.nodejs.org/job/node-test-node-addon-api-new/246/
10	https://ci.nodejs.org/job/node-test-node-addon-api-new/247/
8	https://ci.nodejs.org/job/node-test-node-addon-api-new/248/

gabrielschulhof · 2019-07-17T19:30:17Z

@NickNaso 🤦 can't land this until date is actually backported to v10.x and v8.x. Sorry, I forgot!

NickNaso · 2019-07-17T19:33:55Z

@gabrielschulhof It's ok so it could be a good idea mark this PR with do not land tag and I will provide to link it the PR to the issue where we are tracking the backporting tasks. Do you agree?

gabrielschulhof · 2019-07-17T20:21:55Z

@NickNaso SGTM 👍

mathiask88 · 2019-08-16T20:40:20Z

Any news on this? How long does it usually take until a backport lands? I would like to see this basic functionality implemented in node/napi.

mhdawson · 2019-08-20T21:18:55Z

@mathiask88 V8 is in maintenance so updates are not frequent. @gabrielschulhof with the proper version checks this could probably be landed but only be enable version N-API version >5 right? What we'd want to avoid in that case is it 8.x supporting V 5 before 10.x does.

mhdawson · 2019-09-04T15:38:01Z

@mathiask88 coud you update to add the N-API version checks so that the functionality is only enabled if N-API is set to version 5 or greater?

mathiask88 · 2019-09-04T19:47:45Z

@mhdawson Something like this? I left this line intentionally so that I don't get a linker error when I run npm test with my local node v12. Because without specifying --NAPI_VERSION= the binding.cc defaults to experimental.

mhdawson · 2019-09-04T22:12:37Z

That is what I had in mind. Leaving that line makes sense as well.

CI run on V8.x https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/549/

CI run on V10.x https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/550/

CI run on 12.x - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/551/

CI run on master (13.x) - https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/552/

mhdawson · 2019-09-04T22:27:16Z

I'm confused because it looks like from the CI output the data tests are running on 10 and 12 which I don't expect.

mathiask88 · 2019-09-05T17:19:49Z

Locally I have node 12.10.0 + WIN10 and following tests are running

npm test

Starting test suite

Running test 'arraybuffer'
Running test 'asynccontext'
Running test 'asyncworker'
Running test 'asyncworker-nocallback'
Running test 'asyncworker-persistent'
Running test 'basic_types/array'
Running test 'basic_types/boolean'
Running test 'basic_types/number'
Running test 'basic_types/value'
Running test 'bigint'
Running test 'date'
Running test 'buffer'
Running test 'callbackscope'
Running test 'dataview/dataview'
Running test 'dataview/dataview_read_write'
Running test 'error'
Running test 'external'
Running test 'function'
Running test 'handlescope'
Running test 'memory_management'
Running test 'name'
Running test 'object/delete_property'
Running test 'object/get_property'
Running test 'object/has_own_property'
Running test 'object/has_property'
Running test 'object/object'
Running test 'object/object_deprecated'
Running test 'object/set_property'
Running test 'promise'
Running test 'threadsafe_function/threadsafe_function_ptr'
Running test 'threadsafe_function/threadsafe_function'
Running test 'typedarray'
Running test 'typedarray-bigint'
Running test 'objectwrap'
Running test 'objectreference'
Running test 'version_management'

All tests passed!

npm test --NAPI_VERSION=4

Starting test suite

Running test 'arraybuffer'
Running test 'asynccontext'
Running test 'asyncworker'
Running test 'asyncworker-nocallback'
Running test 'asyncworker-persistent'
Running test 'basic_types/array'
Running test 'basic_types/boolean'
Running test 'basic_types/number'
Running test 'basic_types/value'
Running test 'buffer'
Running test 'callbackscope'
Running test 'dataview/dataview'
Running test 'dataview/dataview_read_write'
Running test 'error'
Running test 'external'
Running test 'function'
Running test 'handlescope'
Running test 'memory_management'
Running test 'name'
Running test 'object/delete_property'
Running test 'object/get_property'
Running test 'object/has_own_property'
Running test 'object/has_property'
Running test 'object/object'
Running test 'object/object_deprecated'
Running test 'object/set_property'
Running test 'promise'
Running test 'threadsafe_function/threadsafe_function_ptr'
Running test 'threadsafe_function/threadsafe_function'
Running test 'typedarray'
Running test 'objectwrap'
Running test 'objectreference'
Running test 'version_management'

All tests passed!

npm test --NAPI_VERSION=5 Does obviously not compile

Is NAPI_VERSION not specified in the CI?

mhdawson · 2019-09-05T20:24:09Z

The CI uses the process.versions to set the --NAPI_VERSION to the one reported by the version of Node.js being tested.

mhdawson · 2019-09-05T20:28:43Z

From the CI job, this is how it gets and sets it.

 echo 'console.log(process.versions.napi)' >get_napi_version.js
    NAPI_VERSION_REPORTED=`node get_napi_version.js`
    npm test --NAPI_VERSION=$NAPI_VERSION_REPORTED

So it should be being set appropriately for the Node.js version. When I looked it was still 4 for 10.x which is why I was confused that it ran in https://ci.nodejs.org/view/x%20-%20Abi%20stable%20module%20API/job/node-test-node-addon-api-new/nodes=rhel72-s390x/550/console

mathiask88 · 2019-09-05T20:48:22Z

@mhdawson Seems like there is a version check on adding --NAPI_VERSION
Have a look at the win10 test on v8.x

+ '[' 8 -gt 9 ']'
+ echo 'console.log(process.versions.napi)'
++ node get_napi_version.js
+ NAPI_VERSION_REPORTED=4
+ npm test --NAPI_VERSION=4

and for v10.x

+ '[' 10 -gt 9 ']'
+ npm test

mhdawson · 2019-09-05T20:58:01Z

@mathiask88 good catch , that explains why they are running. We should leave it that way as it's good to run the tests if we can.

The CI has


  if [ ${NODEJS_MAJOR_VERSION} -gt "9" ]; then
    # These can run experimental tests
    npm test
  else
    echo 'console.log(process.versions.napi)' >get_napi_version.js
    NAPI_VERSION_REPORTED=`node get_napi_version.js`
    npm test --NAPI_VERSION=$NAPI_VERSION_REPORTED
  fi

mhdawson · 2019-09-05T21:00:41Z

CI looks good to me.

PR-URL: #497 Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

mhdawson · 2019-09-05T21:21:24Z

Landed as 6192e70. Thanks @mathiask88

mathiask88 · 2019-09-05T21:36:17Z

Thank you!

PR-URL: nodejs/node-addon-api#497 Reviewed-By: NickNaso <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gabriel Schulhof <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

NickNaso reviewed Jun 15, 2019

View reviewed changes

napi.h Show resolved Hide resolved

napi.h Outdated Show resolved Hide resolved

napi.h Show resolved Hide resolved

NickNaso reviewed Jun 15, 2019

View reviewed changes

napi.h Outdated Show resolved Hide resolved

NickNaso reviewed Jun 17, 2019

View reviewed changes

doc/date.md Outdated Show resolved Hide resolved

doc/date.md Outdated Show resolved Hide resolved

doc/date.md Outdated Show resolved Hide resolved

doc/date.md Show resolved Hide resolved

doc/date.md Outdated Show resolved Hide resolved

doc/date.md Outdated Show resolved Hide resolved

NickNaso reviewed Jun 19, 2019

View reviewed changes

napi.h Outdated Show resolved Hide resolved

mhdawson approved these changes Jun 28, 2019

View reviewed changes

NickNaso approved these changes Jun 28, 2019

View reviewed changes

gabrielschulhof approved these changes Jun 28, 2019

View reviewed changes

addaleax approved these changes Jun 30, 2019

View reviewed changes

NickNaso added the do not land label Jul 17, 2019

NickNaso mentioned this pull request Jul 17, 2019

What should go into N-API 5? nodejs/abi-stable-node#374

Closed

mathiask88 added 2 commits September 4, 2019 21:52

adds experimental napi_date

9e7b962

removes experimental, adds version guard

20fafba

mhdawson removed the do not land label Sep 5, 2019

mhdawson closed this Sep 5, 2019

adds experimental napi_date #497

adds experimental napi_date #497

Conversation

mathiask88 commented Jun 14, 2019

NickNaso left a comment

Choose a reason for hiding this comment

mathiask88 commented Jun 16, 2019

NickNaso left a comment

Choose a reason for hiding this comment

mhdawson commented Jun 17, 2019

mathiask88 commented Jun 17, 2019

mhdawson commented Jun 17, 2019

mhdawson commented Jun 17, 2019

NickNaso left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

mathiask88 commented Jun 28, 2019

mhdawson commented Jun 28, 2019 • edited Loading

NickNaso left a comment

Choose a reason for hiding this comment

gabrielschulhof commented Jun 28, 2019

NickNaso commented Jun 28, 2019

mathiask88 commented Jun 28, 2019

gabrielschulhof commented Jun 28, 2019

NickNaso commented Jul 17, 2019

gabrielschulhof commented Jul 17, 2019

gabrielschulhof commented Jul 17, 2019

gabrielschulhof commented Jul 17, 2019

NickNaso commented Jul 17, 2019 • edited Loading

gabrielschulhof commented Jul 17, 2019

mathiask88 commented Aug 16, 2019

mhdawson commented Aug 20, 2019

mhdawson commented Sep 4, 2019

mathiask88 commented Sep 4, 2019

mhdawson commented Sep 4, 2019

mhdawson commented Sep 4, 2019

mathiask88 commented Sep 5, 2019 • edited Loading

mhdawson commented Sep 5, 2019

mhdawson commented Sep 5, 2019

mathiask88 commented Sep 5, 2019

mhdawson commented Sep 5, 2019

mhdawson commented Sep 5, 2019

mhdawson commented Sep 5, 2019

mathiask88 commented Sep 5, 2019

mhdawson commented Jun 28, 2019 •

edited

Loading

NickNaso commented Jul 17, 2019 •

edited

Loading

mathiask88 commented Sep 5, 2019 •

edited

Loading