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.

Sign up for GitHub

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

Jump to bottom

n-api: mark version 5 N-APIs as stable #29401

Closed
wants to merge 1 commit into from
Closed

n-api: mark version 5 N-APIs as stable #29401

wants to merge 1 commit into from

Conversation

gabrielschulhof
Copy link
Contributor

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

@nodejs-github-bot nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Sep 2, 2019
@gabrielschulhof gabrielschulhof added the node-api Issues and PRs related to the Node-API. label Sep 2, 2019
@gabrielschulhof gabrielschulhof mentioned this pull request Sep 2, 2019
Closed
bnoordhuis
bnoordhuis approved these changes Sep 2, 2019
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but you might want to update the documentation for napi_get_date_value() to specify whether it does or doesn't include leap seconds.

Since it's UNIX time and presumably the equivalent of Date.p.valueOf() I'm guessing it doesn't but there's really no way to tell for a casual reader.

@nodejs-github-bot
Copy link
Collaborator

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

@gabrielschulhof
Copy link
Contributor Author

@bnoordhuis I'm actually not sure whether it does or does not account for leap seconds. napi_get_date_value() merely calls v8::Date::ValueOf() which is documented as a specialization of v8::Value::NumberValue without mention of its (lack of) accounting for leap seconds.

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@gabrielschulhof
Copy link
Contributor Author

Rebased. Hoping this'll fix Travis.

@bnoordhuis
Copy link
Member

I'm actually not sure whether it does or does not account for leap seconds.

From https://tc39.es/ecma262/#sec-time-values-and-time-range:

Time in ECMAScript does not observe leap seconds; they are ignored.

@gabrielschulhof gabrielschulhof mentioned this pull request Sep 4, 2019
Closed
mhdawson
mhdawson approved these changes Sep 4, 2019
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

gabrielschulhof pushed a commit that referenced this pull request Sep 5, 2019
n-api: mark version 5 N-APIs as stable
4e5bb25 
PR-URL: #29401
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof
Copy link
Contributor Author

Landed in 4e5bb25.

gabrielschulhof pushed a commit to gabrielschulhof/node that referenced this pull request Sep 5, 2019
n-api: mark version 5 N-APIs as stable
fc29c08 
PR-URL: nodejs#29401
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@gabrielschulhof gabrielschulhof mentioned this pull request Sep 5, 2019
Closed
4 tasks
@gabrielschulhof gabrielschulhof mentioned this pull request Sep 20, 2019
Closed
3 tasks
targos pushed a commit that referenced this pull request Sep 20, 2019
@targos
n-api: mark version 5 N-APIs as stable
50d7c39 
PR-URL: #29401
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@BridgeAR BridgeAR mentioned this pull request Sep 24, 2019
Merged
This was referenced Sep 25, 2019
Closed
Closed
This was referenced Sep 26, 2019
Closed
Closed
Closed
Closed
BethGriggs pushed a commit that referenced this pull request Oct 1, 2019
@BethGriggs
n-api: mark version 5 N-APIs as stable
c74c6a5 
PR-URL: #29401
Backport-PR-URL: #29458
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@BethGriggs BethGriggs mentioned this pull request Oct 7, 2019
Merged
BethGriggs added a commit that referenced this pull request Oct 18, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
44ef30e 
Notable changes:

- **deps**: upgrade openssl sources to 1.1.1d (Sam Roberts)
  [#29921](#29921)
- **dns**: remove dns.promises experimental warning (cjihrig)
  [#26592](#26592)
- **fs**: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581](#26581)
- **n-api**: mark version 5 N-APIs as stable (Gabriel Schulhof)
  [#29401](#29401)
- **stream**: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 19, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
c3e596f 
Notable changes:

- **deps**: update npm to 6.11.3 (claudiahdz)
  [#29430](#29430)
- **deps**: upgrade openssl sources to 1.1.1d (Sam Roberts)
  [#29921](#29921)
- **dns**: remove dns.promises experimental warning (cjihrig)
  [#26592](#26592)
- **fs**: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581](#26581)
- **n-api**: mark version 5 N-APIs as stable (Gabriel Schulhof)
  [#29401](#29401)
- **stream**: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 21, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
ae884d3 
Notable changes:

- **deps**: update npm to 6.11.3 (claudiahdz)
  [#29430](#29430)
- **deps**: upgrade openssl sources to 1.1.1d (Sam Roberts)
  [#29921](#29921)
- **dns**: remove dns.promises experimental warning (cjihrig)
  [#26592](#26592)
- **fs**: remove experimental warning for fs.promises (Anna Henningsen)
  [#26581](#26581)
- **n-api**: mark version 5 N-APIs as stable (Gabriel Schulhof)
  [#29401](#29401)
- **stream**: make Symbol.asyncIterator support stable (Matteo Collina)
  [#26989](#26989)

PR-URL: #29875
BethGriggs added a commit that referenced this pull request Oct 22, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
5dae3ef 
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
BethGriggs added a commit that referenced this pull request Oct 22, 2019
@BethGriggs
2019-10-22, Version 10.17.0 'Dubnium' (LTS)
afbbcb0 
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
@gabrielschulhof gabrielschulhof mentioned this pull request Mar 14, 2020
Closed
4 tasks
@gabrielschulhof gabrielschulhof deleted the mark-apis-as-stable branch April 21, 2020 02:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnoordhuis bnoordhuis bnoordhuis approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

@mhdawson mhdawson mhdawson approved these changes

@addaleax addaleax Awaiting requested review from addaleax

Assignees
No one assigned
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. node-api Issues and PRs related to the Node-API.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@gabrielschulhof @nodejs-github-bot @bnoordhuis @jasnell @cjihrig @mhdawson