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: implement date object #25917

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
9 participants
@jarrodconnolly
Copy link
Contributor

jarrodconnolly commented Feb 4, 2019

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
@jarrodconnolly

This comment has been minimized.

Copy link
Contributor Author

jarrodconnolly commented Feb 4, 2019

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.

@jarrodconnolly jarrodconnolly force-pushed the jarrodconnolly:napi-date branch from 4507aa5 to cb37fcb Feb 5, 2019

@addaleax
Copy link
Member

addaleax left a comment

Show resolved Hide resolved src/js_native_api_v8.cc Outdated
#include <js_native_api.h>
#include "../common.h"

static napi_value createDate(napi_env env, napi_callback_info info) {

This comment has been minimized.

@addaleax

addaleax Feb 6, 2019

Member

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 :)

This comment has been minimized.

@jarrodconnolly

jarrodconnolly Feb 7, 2019

Author Contributor

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) {

This comment has been minimized.

@mhdawson

mhdawson Feb 7, 2019

Member

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

This comment has been minimized.

@addaleax

addaleax Feb 9, 2019

Member

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

@addaleax

This comment has been minimized.

Show resolved Hide resolved doc/api/n-api.md Outdated
@mhdawson
Copy link
Member

mhdawson left a comment

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

Show resolved Hide resolved doc/api/n-api.md Outdated
@mhdawson
Copy link
Member

mhdawson left a comment

LGTM

@addaleax

This comment has been minimized.

@cjihrig
Copy link
Contributor

cjihrig left a comment

LGTM with a few tiny nits.

Show resolved Hide resolved doc/api/n-api.md Outdated
Show resolved Hide resolved doc/api/n-api.md Outdated
Show resolved Hide resolved doc/api/n-api.md Outdated

jarrodconnolly added some commits Feb 4, 2019

n-api: implement date object
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
Add the `napi_get_date_value` method to get the original
time value from the Date object.

@jarrodconnolly jarrodconnolly force-pushed the jarrodconnolly:napi-date branch from 76d14ff to c21826f Feb 12, 2019

@jarrodconnolly

This comment has been minimized.

Copy link
Contributor Author

jarrodconnolly commented Feb 14, 2019

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.

@jarrodconnolly

This comment has been minimized.

Copy link
Contributor Author

jarrodconnolly commented Feb 17, 2019

@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.

@vsemozhetbyt

This comment has been minimized.

@jarrodconnolly

This comment has been minimized.

Copy link
Contributor Author

jarrodconnolly commented Feb 18, 2019

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.
@addaleax

This comment has been minimized.

Copy link
Member

addaleax commented Feb 18, 2019

@danbev

This comment has been minimized.

Copy link
Member

danbev commented Feb 20, 2019

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

@jarrodconnolly

This comment has been minimized.

Copy link
Contributor Author

jarrodconnolly commented Feb 20, 2019

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.

@jarrodconnolly

This comment has been minimized.

Copy link
Contributor Author

jarrodconnolly commented Feb 25, 2019

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

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Feb 26, 2019

@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.

@cjihrig

This comment has been minimized.

Copy link
Contributor

cjihrig commented Feb 26, 2019

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

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Feb 28, 2019

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/

@mhdawson

This comment has been minimized.

Copy link
Member

mhdawson commented Feb 28, 2019

Lite CI good Landed in 13b1aaf

@mhdawson mhdawson closed this Feb 28, 2019

mhdawson added a commit that referenced this pull request Feb 28, 2019

n-api: implement date object
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>

addaleax added a commit that referenced this pull request Mar 1, 2019

n-api: implement date object
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>

@BridgeAR BridgeAR referenced this pull request Mar 4, 2019

Merged

v11.11.0 proposal #26322

BridgeAR added a commit that referenced this pull request Mar 5, 2019

2019-03-05, Version 11.11.0 (Current)
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)

BridgeAR added a commit that referenced this pull request Mar 5, 2019

2019-03-05, Version 11.11.0 (Current)
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)

BridgeAR added a commit that referenced this pull request Mar 5, 2019

2019-03-06, Version 11.11.0 (Current)
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)

BridgeAR added a commit that referenced this pull request Mar 5, 2019

2019-03-06, Version 11.11.0 (Current)
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
  * Use fake MessageEvent for port.onmessage (Anna Henningsen)
    #26082

BridgeAR added a commit that referenced this pull request Mar 5, 2019

2019-03-06, Version 11.11.0 (Current)
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

Trott pushed a commit to Trott/io.js that referenced this pull request Mar 6, 2019

2019-03-06, Version 11.11.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.