n-api: add code parameter to error helpers

[mhdawson]

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]

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)

[mhdawson]

ok I will update.

[addaleax]

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().

[mhdawson]

Ok sure.

[mhdawson]

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]

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?

[mhdawson]

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]

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]

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

@addaleax, pushed commit to address comments.

[mhdawson]

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

[addaleax]

Thanks for the ping, LGTM!

[addaleax]

I think the ' is a typo here

[jasnell]

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

[mhdawson]

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

[mhdawson]

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]

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

[mhdawson]

needs a rebase now doing that.

[mhdawson]

Simple rebase pushed

[mhdawson]

Going to land.

[mhdawson]

Landed as ac41db4

[jasongin]

@mhdawson Did you forget to close this?

[addaleax]

I think so. :)