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

Add logic to handle graceful error handling when primitive errors are thrown by JS functions #1075

Merged
merged 7 commits into from
Nov 26, 2021
74 changes: 56 additions & 18 deletions napi-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -2603,20 +2603,45 @@ inline Error::Error(napi_env env, napi_value value) : ObjectReference(env, nullp

NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_create_object");

status = napi_set_property(env,
wrappedErrorObj,
String::From(env, "errorVal"),
Value::From(env, value));
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_set_property");

status = napi_set_property(
env,
wrappedErrorObj,
String::From(env,
"4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject"),
Value::From(env, value));
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_set_property");
napi_property_descriptor errValDesc = {
"errorVal", // const char* utf8Name
String::From(env, "errorVal"), // napi_value name
Copy link
Contributor

Choose a reason for hiding this comment

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

This can be nullptr. utf8name is sufficient for specifying the name of the property.

nullptr, // Method
nullptr, // getter
nullptr, // setter
Value::From(env, value), // napi_value value
napi_enumerable,
nullptr};

status = napi_define_properties(env, wrappedErrorObj, 1, &errValDesc);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_define_properties");

MaybeOrValue<Symbol> result = Symbol::For(env, "isWrapObject");
napi_property_descriptor wrapObjFlag = {nullptr,
nullptr,
nullptr,
nullptr,
nullptr,
Value::From(env, value),
napi_enumerable,
nullptr};

#ifdef NODE_ADDON_API_ENABLE_MAYBE
Symbol uniqueSymb;
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Move this declaration outside the #ifdef.

if (result.IsJust()) {
uniqueSymb = result.Unwrap();
}

wrapObjFlag.name = uniqueSymb;
status = napi_define_properties(env, wrappedErrorObj, 1, &wrapObjFlag);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_define_properties");
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Remove these two lines.

#else

wrapObjFlag.name = result;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
wrapObjFlag.name = result;
uniqueSymb = result;
wrapObjFlag.name = result;

status = napi_define_properties(env, wrappedErrorObj, 1, &wrapObjFlag);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_define_properties");
Comment on lines +2620 to +2621
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Move these two statements below the #endif. That way, you are not duplicating them in both bodies of the #if...#else...#endif, thereby reducing the size of the the #if...#else...#endif block.


#endif
status = napi_create_reference(env, wrappedErrorObj, 1, &_ref);
}

Expand All @@ -2642,18 +2667,31 @@ inline Object Error::Value() const {
if (type != napi_symbol) {
// We are checking if the object is wrapped
bool isWrappedObject = false;
napi_has_property(
_env,
refValue,
String::From(_env, "4b3d96fd-fb87-4951-a979-eb4f9d2f2ce9-isWrapObject"),
&isWrappedObject);

MaybeOrValue<Symbol> result = Symbol::For(_env, "isWrapObject");
Copy link
Member

Choose a reason for hiding this comment

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

One suggestion is maybe we should still use a GUID here instead of the string.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be guarded by #ifdef NODE_ADDON_API_ENABLE_MAYBE since it's MaybeOrValue<Symbol>?


#ifdef NODE_ADDON_API_ENABLE_MAYBE
Symbol uniqueSymb;
if (result.IsJust()) {
uniqueSymb = result.Unwrap();
}
status = napi_has_property(_env, refValue, uniqueSymb, &isWrappedObject);
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the same pattern here as I mentioned before. Move Symbol uniqueSymb outside the #ifdef ... #else ... #endif block and place the common code after the block so you don't have to write it twice.

NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_set_property");

#else

status = napi_has_property(_env, refValue, result, &isWrappedObject);
NAPI_FATAL_IF_FAILED(status, "Error::Error", "napi_set_property");
#endif

// Don't care about status

if (isWrappedObject == true) {
napi_value unwrappedValue;
status = napi_get_property(
_env, refValue, String::From(_env, "errorVal"), &unwrappedValue);
NAPI_THROW_IF_FAILED(_env, status, Object());

return Object(_env, unwrappedValue);
}
}
Expand Down