n-api: add code parameter to error helpers #13988

mhdawson · 2017-06-29T15:42:59Z

In support of the effort to add error codes to all errors
generated by Node.js, add an optional code parameter to the
helper functions used to throw/create errors in N-API.

Fixes: #13933

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

Affected core subsystem(s)

n-api

This does assume that we are ok changing the existing APIs, that will require that we update the wrapper and our ports. The alternative would be to add new functions (maybe _with_code) instead. I lean towards the replace as it does the most to encourage the use of the code, but do want everybody's feedback on the right way to go at this point.

addaleax

Left a few comments, but generally thanks for picking this up!

I am not sure what to do about the API breakage either.

addaleax · 2017-06-29T18:06:28Z

src/node_api.cc

+static napi_status set_error_code(napi_env env,
+                                  napi_value code,
+                                  const char* code_cstring,
+                                  v8::Local<v8::Value> val) {


nit: I think this might be clearer if val were the second parameter, that’s what I would expect for a setting operation (I’d also call it obj, or even error – val is pretty generic)

ok I will update.

addaleax · 2017-06-29T18:06:29Z

src/node_api.cc

+    v8::Isolate* isolate = env->isolate;
+    v8::Local<v8::Context> context = isolate->GetCurrentContext();
+
+    v8::MaybeLocal<v8::Object> maybe_obj = val->ToObject(context);


nit: I’d prefer if this was a val->IsObject() check + casting the handle via .As<Object>() rather than doing a possible conversion with ToObject().

I don't think we even need the IsObject() check since for all of the callers we know we are passing in an Object so that does make the code more compact.

addaleax · 2017-06-29T18:14:50Z

src/node_api.cc

-    napi_throw_type_error(env, "constructor must be a function");
+    napi_throw_type_error(env,
+                          "ERR_NAPI_CONS_FUNCTION",
+                          "TypeError [Constructor must be a function]");


I think it gets a bit tricky here. If I read this correctly, the resulting error will look something like TypeError: TypeError [Constructor must be a function] when stringified; if this were an internal error thrown from JS, it should look something like TypeError[ERR_NAPI_CONS_FUNCTION]: Constructor must be a function.

Node’s internal errors do getter magic that would not be trivial to add here, but can I suggest that to make them look more alike, set_error_code also sets err.name to (Type|Range)Error[$code] in addition to setting err.code?

Just realized that I had these wrong so came back to take a look and see you already caught it:). I had meant to make them look the same ie as you have shown 'TypeError [ERR_NAPI_CONS_FUNCTION] Constructor must be a function'. but got it wrong. Will fix it up.

kkoopa · 2017-06-29T20:56:26Z

Why are some error code parameters napi_value and others char *? I would have expected error codes to be integer.

Regarding breakage at this point, go for it. That is after all the point of having an experimental API and improving it.

mhdawson · 2017-06-29T22:36:15Z

@kkoopa, the codes are strings as defined in the approach outlined in #11273

I stuck with keeping the parameters consistent with the helpers.

The throw_ ones take a cstring to make it easier on the callers avoiding the extra call to create a napi_value. In that case I kept the code to a cstring in that case as well.

The create_ helpers used napi_value. napi_value is more generic as it support different types of strings (utf8, latin1, etc.) so I kept that flexibility of a napi_value for the code as well since it was already there
for the message (ie if you have to create one napi-value then why not 2)

jasnell

Largely rubber stamp LGTM

mhdawson · 2017-06-30T21:43:04Z

@addaleax, pushed commit to address comments.

mhdawson · 2017-07-07T17:13:34Z

@addaleax wondering if you are going to be able to take another look.

addaleax

Thanks for the ping, LGTM!

addaleax · 2017-07-07T17:16:11Z

doc/api/n-api.md

+is 'ERR_ERROR_1' and a TypeError is being created the name will be:
+
+```
+TypeError [ERR_ERROR_1']


I think the ' is a typo here

jasnell · 2017-07-10T16:17:35Z

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

mhdawson · 2017-07-11T21:39:40Z

Fixed typo, new CI run: https://ci.nodejs.org/job/node-test-pull-request/9073/console

mhdawson · 2017-07-12T20:22:37Z

CI test failures are all the known issues after snapshots were turned off. I thin this is ready to land.

NOT marking as SemVer major since N-API is still experimental.

mhdawson · 2017-07-12T21:18:25Z

PR to match up the wrapper in order to minimize time wrapper is broken: nodejs/node-addon-api#78

mhdawson · 2017-07-13T18:37:09Z

needs a rebase now doing that.

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. Fixes: nodejs#13933

mhdawson · 2017-07-13T18:49:45Z

Simple rebase pushed

mhdawson · 2017-07-13T18:58:18Z

Going to land.

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. PR-URL: #13988 Fixes: #13933 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

mhdawson · 2017-07-13T19:16:13Z

Landed as ac41db4

jasongin · 2017-07-24T16:36:40Z

@mhdawson Did you forget to close this?

addaleax · 2017-07-24T16:38:46Z

I think so. :)

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. PR-URL: nodejs#13988 Fixes: nodejs#13933 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. Backport-PR-URL: nodejs#14453 Backport-Reviewed-By: James M Snell <jasnell@gmail.com> PR-URL: nodejs#13988 Fixes: nodejs#13933 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu * [gabrielschulhof](https://github.com/gabrielschulhof) – Gabriel Schulhof

V8 6.0: The V8 engine has been upgraded to version 6.0, which has a significantly changed performance profile. [#14574](#14574) More detailed information on performance differences can be found at https://medium.com/the-node-js-collection/get-ready-a-new-v8-is-coming-node-js-performance-is-changing-46a63d6da4de Other notable changes: * **DNS** * Independent DNS resolver instances are supported now, with support for cancelling the corresponding requests. [#14518](#14518) * **N-API** * Multiple N-API functions for error handling have been changed to support assigning error codes. [#13988](#13988) * **REPL** * Autocompletion support for `require()` has been improved. [#14409](#14409) * **Utilities** * The WHATWG Encoding Standard (`TextDecoder` and `TextEncoder`) has been implemented as an experimental feature. [#13644](#13644) * **Added new collaborators** * [XadillaX](https://github.com/XadillaX) – Khaidi Chu * [gabrielschulhof](https://github.com/gabrielschulhof) – Gabriel Schulhof Conflicts: src/node_version.h

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. PR-URL: nodejs#13988 Fixes: nodejs#13933 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. Backport-PR-URL: #19447 PR-URL: #13988 Fixes: #13933 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. errors Issues and PRs related to JavaScript errors originated in Node.js core. node-api Issues and PRs related to the Node-API. labels Jun 29, 2017

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jun 29, 2017

addaleax reviewed Jun 29, 2017

View reviewed changes

jasnell approved these changes Jun 30, 2017

View reviewed changes

addaleax approved these changes Jul 7, 2017

View reviewed changes

mhdawson mentioned this pull request Jul 12, 2017

Update to new error helper signatures in n-api nodejs/node-addon-api#78

Closed

n-api: add code parameter to error helpers

28ea89f

In support of the effort to add error codes to all errors generated by Node.js, add an optional code parameter to the helper functions used to throw/create errors in N-API. Fixes: nodejs#13933

mhdawson force-pushed the napi-error-code branch from adf6532 to 28ea89f Compare July 13, 2017 18:49

addaleax closed this Jul 24, 2017

addaleax mentioned this pull request Jul 24, 2017

[v8.x] n-api: add code parameter to error helpers #14453

Closed

addaleax added the backported-to-v8.x label Jul 24, 2017

addaleax mentioned this pull request Aug 2, 2017

v8.3.0 proposal #14594

Merged

addaleax added the notable-change PRs with changes that should be highlighted in changelogs. label Aug 3, 2017

digitalinfinity mentioned this pull request Aug 18, 2017

test: verify detail exception messages in addons-napi/test_constructor #14318

Closed

2 tasks

digitalinfinity mentioned this pull request Aug 31, 2017

Error APIs should make it easy to supply an error code property nodejs/abi-stable-node#241

Closed

MylesBorins added the dont-land-on-v6.x label Jan 11, 2018

MylesBorins mentioned this pull request Apr 16, 2018

v6.14.2 proposal #19996

Merged

mhdawson deleted the napi-error-code branch September 30, 2019 13:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

n-api: add code parameter to error helpers #13988

n-api: add code parameter to error helpers #13988

mhdawson commented Jun 29, 2017 •

edited

Loading

addaleax left a comment

addaleax Jun 29, 2017

mhdawson Jun 29, 2017

addaleax Jun 29, 2017

mhdawson Jun 29, 2017

mhdawson Jun 30, 2017

addaleax Jun 29, 2017

mhdawson Jun 29, 2017 •

edited

Loading

kkoopa commented Jun 29, 2017

mhdawson commented Jun 29, 2017

jasnell left a comment

mhdawson commented Jun 30, 2017

mhdawson commented Jul 7, 2017

addaleax left a comment

addaleax Jul 7, 2017

jasnell commented Jul 10, 2017

mhdawson commented Jul 11, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 13, 2017

mhdawson commented Jul 13, 2017

mhdawson commented Jul 13, 2017

mhdawson commented Jul 13, 2017

jasongin commented Jul 24, 2017

addaleax commented Jul 24, 2017

n-api: add code parameter to error helpers #13988

n-api: add code parameter to error helpers #13988

Conversation

mhdawson commented Jun 29, 2017 • edited Loading

Checklist

Affected core subsystem(s)

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jun 29, 2017

Choose a reason for hiding this comment

mhdawson Jun 29, 2017

Choose a reason for hiding this comment

addaleax Jun 29, 2017

Choose a reason for hiding this comment

mhdawson Jun 29, 2017

Choose a reason for hiding this comment

mhdawson Jun 30, 2017

Choose a reason for hiding this comment

addaleax Jun 29, 2017

Choose a reason for hiding this comment

mhdawson Jun 29, 2017 • edited Loading

Choose a reason for hiding this comment

kkoopa commented Jun 29, 2017

mhdawson commented Jun 29, 2017

jasnell left a comment

Choose a reason for hiding this comment

mhdawson commented Jun 30, 2017

mhdawson commented Jul 7, 2017

addaleax left a comment

Choose a reason for hiding this comment

addaleax Jul 7, 2017

Choose a reason for hiding this comment

jasnell commented Jul 10, 2017

mhdawson commented Jul 11, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 12, 2017

mhdawson commented Jul 13, 2017

mhdawson commented Jul 13, 2017

mhdawson commented Jul 13, 2017

mhdawson commented Jul 13, 2017

jasongin commented Jul 24, 2017

addaleax commented Jul 24, 2017

mhdawson commented Jun 29, 2017 •

edited

Loading

mhdawson Jun 29, 2017 •

edited

Loading