-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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
doc: suggest not to throw JS errors from C++ #18149
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,7 +20,8 @@ | |
* [Others](#others) | ||
* [Type casting](#type-casting) | ||
* [Do not include `*.h` if `*-inl.h` has already been included](#do-not-include-h-if--inlh-has-already-been-included) | ||
* [Avoid throwing JavaScript errors in nested C++ methods](#avoid-throwing-javascript-errors-in-nested-c-methods) | ||
* [Avoid throwing JavaScript errors in C++ methods](#avoid-throwing-javascript-errors-in-c) | ||
* [Avoid throwing JavaScript errors in nested C++ methods](#avoid-throwing-javascript-errors-in-nested-c-methods) | ||
|
||
Unfortunately, the C++ linter (based on | ||
[Google’s `cpplint`](https://github.com/google/styleguide)), which can be run | ||
|
@@ -213,12 +214,64 @@ instead of | |
#include "util-inl.h" | ||
``` | ||
|
||
## Avoid throwing JavaScript errors in nested C++ methods | ||
## Avoid throwing JavaScript errors in C++ | ||
|
||
If you need to throw JavaScript errors from a C++ binding method, try to do it | ||
at the top level and not inside of nested calls. | ||
When there is a need to throw errors from a C++ binding method, try to | ||
return the data necessary for constructing the errors to JavaScript, | ||
then construct and throw the errors [using `lib/internal/errors.js`][errors]. | ||
|
||
A lot of code inside Node.js is written so that typechecking etc. is performed | ||
in JavaScript. | ||
Note that in general, type-checks on arguments should done in JavaScript | ||
before the arguments are passed into C++. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps add something about including a |
||
|
||
If the return value of the binding cannot be used to signal failures or return | ||
the necessary data for constructing errors in JavaScript, pass a context object | ||
to the binding and put the necessary data inside in C++. For example: | ||
|
||
```c++ | ||
void Foo(const FunctionCallbackInfo<Value>& args) { | ||
Environment* env = Environment::GetCurrent(args); | ||
// Let the JavaScript handle the actual type-checking, | ||
// only assertions are placed in C++ | ||
CHECK_EQ(args.Length(), 2); | ||
CHECK(args[0]->IsString()); | ||
CHECK(args[1]->IsObject()); | ||
|
||
int err = DoSomethingWith(args[0].As<String>()); | ||
if (err) { | ||
// Put the data inside the error context | ||
Local<Object> ctx = args[1].As<Object>(); | ||
Local<String> key = FIXED_ONE_BYTE_STRING(env->isolate(), "code") | ||
ctx->Set(env->context(), key, err); | ||
} else { | ||
args.GetReturnValue().Set(something_to_return); | ||
} | ||
} | ||
|
||
// In the initialize function | ||
env->SetMethod(target, "foo", Foo); | ||
``` | ||
|
||
```js | ||
exports.foo = function(str) { | ||
// Prefer doing the type-checks in JavaScript | ||
if (typeof str !== 'string') { | ||
throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'str', 'string'); | ||
} | ||
|
||
const ctx = {}; | ||
const result = binding.foo(str, ctx); | ||
if (ctx.code !== undefined) { | ||
throw errors.Error('ERR_ERROR_NAME', ctx.code); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing |
||
} | ||
return result; | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A nit: missing semicolon. |
||
``` | ||
|
||
### Avoid throwing JavaScript errors in nested C++ methods | ||
|
||
When you have to throw the errors from C++, try to do it at the top level and | ||
not inside of nested calls. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Maybe" mention There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm I think we could make a separate section for the Maybes and link from here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Either is fine. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @TimothyGu Since this was how it's documented prior to this PR, I'll open a separate PR for the usage of |
||
|
||
Using C++ `throw` is not allowed. | ||
|
||
[errors]: https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@targos Fixed, thanks for catching that!