n-api: implement date object by jarrodconnolly · Pull Request #25917 · nodejs/node · GitHub

Implements napi_create_date() as well as napi_is_date() to
allow working with JavaScript Date objects.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

I am working on adding napi_get_date_value to call the underlying V8 ValueOf() to get the value back out of the Date object.

@nodejs/n-api

I’m not sure if we follow a convention for these names in the N-API tests, but elsewhere in the code this would be either create_date or CreateDate :)

I was torn on this one as I saw a mix of case usage for the function names in the tests.

static napi_value createError(napi_env env, napi_callback_info info) {
static napi_value createPromise(napi_env env, napi_callback_info info) {

static napi_value CreateTypedArray(napi_env env, napi_callback_info info) {
static napi_value CreateDataView(napi_env env, napi_callback_info info) {

I don't think we have any different convention for N-API.

Yeah, it’s not like it matters much – the latter style would be more consistent with the rest of core, that’s all.

CI: https://ci.nodejs.org/job/node-test-pull-request/20621/

@jarrodconnolly thanks for the PR. Left a couple of small comments.

LGTM

CI: https://ci.nodejs.org/job/node-test-pull-request/20690/

LGTM with a few tiny nits.

Implements `napi_create_date()` as well as `napi_is_date()` to allow working with JavaScript Date objects.

Add the `napi_get_date_value` method to get the original time value from the Date object.

All changes are complete for this PR including requested changes from the reviews.

I may try a PR at a later date to address the function naming convention across the n-api tests to see if there is interest in standardizing those with the rest of the coding standards.

@addaleax Can I get a CI run on this now that the changes are all complete and maybe an author ready label if the run is smooth? Let me know if there is anything else I need to do, thanks again for the help on this pr.

CI: https://ci.nodejs.org/job/node-test-pull-request/20825/

Thanks for the CI run @vsemozhetbyt looks like it might be an unrelated failure.

The smartos17-64 tests timed out after an exception.

18:28:34 ok 1853 parallel/test-tls-sni-server-client
18:28:34   ---
18:28:34   duration_ms: 1.155
18:28:34   severity: ok
18:28:34   stack: |-
18:28:34 Exception in thread Thread-1:
18:28:34 Traceback (most recent call last):
18:28:34   File "/opt/local/lib/python2.7/threading.py", line 801, in __bootstrap_inner
18:28:34     self.run()
18:28:34   File "/opt/local/lib/python2.7/threading.py", line 754, in run
18:28:34     self.__target(*self.__args, **self.__kwargs)
18:28:34   File "tools/test.py", line 192, in RunSingle
18:28:34     self.HasRun(output)
18:28:34   File "tools/test.py", line 361, in HasRun
18:28:34     self._printDiagnostic()
18:28:34   File "tools/test.py", line 289, in _printDiagnostic
18:28:34     for l in self.traceback.splitlines():
18:28:34 AttributeError: 'list' object has no attribute 'splitlines'
18:28:34 
18:43:34 Build timed out (after 15 minutes). Marking the build as failed.

Resume CI: https://ci.nodejs.org/job/node-test-pull-request/20876/

Should this be land in a single commit or in two?

I believe it should be a single commit. I am guessing you are referring to the second commit which adds the napi_get_date_value. This was just something I thought to add after the original commit to round out the implementation.

Those are just my thoughts, others may have opinions on the regular procedure though.

Just pinging to see if it would be possible to land this PR. My first and I am looking to complete the cycle once before moving on to future PRs.
@addaleax

@jarrodconnolly at this point the CI is locked down due to the security release. If we don't land in the next few days after that release goes out please remind us and I'll try to get it landed.

FWIW, the last CI run was green, so this should be able to land anytime.

Lite CI since its low overhead just to double check nothing changed since the last CI https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/2720/

Lite CI good Landed in 13b1aaf

Implements `napi_create_date()` as well as `napi_is_date()` to allow working with JavaScript Date objects. PR-URL: #25917 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes * n-api: * Implement date object (Jarrod Connolly) (#25917) * util: * Group array elements together (Ruben Bridgewater) (#26269) * Add compact depth mode (Ruben Bridgewater) (#26269) * worker: * Improve integration with native addons (Anna Henningsen) (#26175)

Notable Changes * n-api: * Implement date object (Jarrod Connolly) #25917 * util: * Add compact depth mode for `util.inspect()` (Ruben Bridgewater) #26269 * worker: * Improve integration with native addons (Anna Henningsen) #26175 * MessagePort.prototype.onmessage takes arguments closer to the Web specification now (Anna Henningsen) #26082

Notable Changes * n-api: * Implement date object (Jarrod Connolly) nodejs#25917 * util: * Add compact depth mode for `util.inspect()` (Ruben Bridgewater) nodejs#26269 * worker: * Improve integration with native addons (Anna Henningsen) nodejs#26175 * MessagePort.prototype.onmessage takes arguments closer to the Web specification now (Anna Henningsen) nodejs#26082

Implements `napi_create_date()` as well as `napi_is_date()` to allow working with JavaScript Date objects. PR-URL: nodejs#25917 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Implements `napi_create_date()` as well as `napi_is_date()` to allow working with JavaScript Date objects. Backport-PR-URL: #28298 PR-URL: #25917 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable changes: * crypto: * add support for chacha20-poly1305 for AEAD (chux0519) #24081 * increase maxmem range from 32 to 53 bits (Tobias Nießen) #28799 * deps: * update npm to 6.11.3 (claudiahdz) #29430 * upgrade openssl sources to 1.1.1d (Sam Roberts) #29921 * dns: * remove dns.promises experimental warning (cjihrig) #26592 * fs: * remove experimental warning for fs.promises (Anna Henningsen) #26581 * http: * makes response.writeHead return the response (Mark S. Everitt) #25974 * http2: * makes response.writeHead return the response (Mark S. Everitt) #25974 * n-api: * make func argument of napi\_create\_threadsafe\_function optional (legendecas) #27791 * mark version 5 N-APIs as stable (Gabriel Schulhof) #29401 * implement date object (Jarrod Connolly) #25917 * process: * add --unhandled-rejections flag (Ruben Bridgewater) #26599 * stream: * implement Readable.from async iterator utility (Guy Bedford) #27660 * make Symbol.asyncIterator support stable (Matteo Collina) #26989 PR-URL: #29875

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Feb 4, 2019

vsemozhetbyt added the node-api Issues and PRs related to the Node-API. label Feb 4, 2019

jarrodconnolly force-pushed the napi-date branch from 4507aa5 to cb37fcb Compare February 5, 2019 06:33

addaleax approved these changes Feb 6, 2019

View reviewed changes

mhdawson reviewed Feb 7, 2019

View reviewed changes

Comment thread doc/api/n-api.md Outdated

mhdawson reviewed Feb 7, 2019

View reviewed changes

vsemozhetbyt reviewed Feb 7, 2019

View reviewed changes

Comment thread doc/api/n-api.md Outdated

mhdawson approved these changes Feb 8, 2019

View reviewed changes

cjihrig approved these changes Feb 11, 2019

View reviewed changes

Comment thread doc/api/n-api.md Outdated

Comment thread doc/api/n-api.md Outdated

Comment thread doc/api/n-api.md Outdated

jarrodconnolly added 5 commits February 11, 2019 21:43

n-api: implement date object

cde1ce1

Implements `napi_create_date()` as well as `napi_is_date()` to allow working with JavaScript Date objects.

n-api: add method to get date value

3a4af6e

Add the `napi_get_date_value` method to get the original time value from the Date object.

naming convention changes from pr review

06f7337

documentation changes from pr review

bb48cac

grammar changes and setting napiVersion to 4 from pr review

c21826f

jarrodconnolly force-pushed the napi-date branch from 76d14ff to c21826f Compare February 12, 2019 05:44

jasnell approved these changes Feb 18, 2019

View reviewed changes

BridgeAR added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Feb 20, 2019

mhdawson closed this Feb 28, 2019

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 1, 2019

BridgeAR mentioned this pull request Mar 4, 2019

v11.11.0 proposal #26322

Merged

gabrielschulhof mentioned this pull request Jun 19, 2019

[v10.x] n-api: implement date object #28298

Closed

4 tasks

gabrielschulhof mentioned this pull request Jun 19, 2019

[v8.x] n-api: implement date object #28301

Closed

4 tasks

BethGriggs added the lts-watch-v10.x label Aug 20, 2019

BethGriggs removed the lts-watch-v10.x label Oct 7, 2019

BethGriggs mentioned this pull request Oct 7, 2019

v10.17.0 proposal #29875

Merged

McFlyssss mentioned this pull request Dec 22, 2020

[v3.7.0-alpha.2] CreatedAt must be of type 'date', got 'object' realm/realm-js#2755

Closed

n-api: implement date object by jarrodconnolly · Pull Request #25917 · nodejs/node · GitHub

Uh oh!

Uh oh!

n-api: implement date object - #25917

n-api: implement date object#25917jarrodconnolly wants to merge 5 commits intonodejs:masternodejs/node:masterfrom jarrodconnolly:napi-datejarrodconnolly/node:napi-dateCopy head branch name to clipboard

Conversation

jarrodconnolly commented Feb 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

jarrodconnolly commented Feb 4, 2019

Uh oh!

addaleax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

addaleax Feb 6, 2019

Choose a reason for hiding this comment

Uh oh!

jarrodconnolly Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

mhdawson Feb 7, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax Feb 9, 2019

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 6, 2019

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mhdawson left a comment

Choose a reason for hiding this comment

Uh oh!

addaleax commented Feb 9, 2019

Uh oh!

cjihrig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jarrodconnolly commented Feb 14, 2019

Uh oh!

jarrodconnolly commented Feb 17, 2019

Uh oh!

vsemozhetbyt commented Feb 17, 2019

Uh oh!

jarrodconnolly commented Feb 18, 2019

Uh oh!

addaleax commented Feb 18, 2019

Uh oh!

danbev commented Feb 20, 2019

Uh oh!

jarrodconnolly commented Feb 20, 2019

Uh oh!

jarrodconnolly commented Feb 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mhdawson commented Feb 26, 2019

Uh oh!

cjihrig commented Feb 26, 2019

Uh oh!

mhdawson commented Feb 28, 2019

Uh oh!

mhdawson commented Feb 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

n-api: implement date object#25917
jarrodconnolly wants to merge 5 commits into
nodejs:masterfrom
jarrodconnolly:napi-date

jarrodconnolly commented Feb 4, 2019 •
edited
Loading

jarrodconnolly commented Feb 25, 2019 •
edited
Loading