Documentation for error handling by NickNaso · Pull Request #259 · nodejs/node-addon-api

NickNaso · 2018-05-03T02:06:13Z

First step on writing documentation for error handling.

Changed version of node-addon-api on the package.json example see: [issue 206](#206 (comment))

kfarnung · 2018-05-03T17:53:25Z

+section).
+
+If C++ exceptions are enabled (for more info see: [Setup](setup.md) section),
+then the **Error** class extends ```std::exception``` and enables integrated


For single line code I think you can just use a single backtick.

I will provide to remove where necessary.

kfarnung · 2018-05-03T17:55:32Z

+
+### Constructor
+
+Initializes a ```Error``` instance from an existing ```Error``` object.


I've been wresting with how to describe this, maybe "from an existing JavaScript Error object." The description as it stands sounds more like a copy constructor when it seems like this would be used more for JavaScript objects which have been acquired elsewhere.

kfarnung · 2018-05-03T17:56:18Z

+void ThrowAsJavaScriptException() const;
+```
+
+Throw the error as JavaScript exception.


Nit: I would move this above where the description normally goes and then update it to say "Throws"

kfarnung · 2018-05-03T17:56:42Z

+```
+
+Returns a pointer to a null-terminated string that is used to identify the
+exception. This method can be used only if the eceptions mechanis is enabled.


nit: "exception mechanism"

I think this file is also missing

static NAPI_NO_RETURN void Fatal(const char* location, const char* message);

kfarnung · 2018-05-03T17:57:39Z


-You are reading a draft of the next documentation and it's in continuos update so
-if you don't find what you need please refer to:
-[C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/)


What is our feeling about this? I've seen some of the documentation updates leave it, but maybe it's the right time to start removing these designations.

/cc @mhdawson

That was my thought, once we fill in the section lets remove the comment.

Replied above at this point I'd say take it out.

kfarnung · 2018-05-03T17:58:52Z

-You are reading a draft of the next documentation and it's in continuos update so
-if you don't find what you need please refer to:
-[C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/)
+The error handling represents one of the most important thing on implementing a


nit: "Error handling represents one of the most important considerations when implementing a Node.js native add-on."

kfarnung · 2018-05-03T18:00:13Z

+The error handling represents one of the most important thing on implementing a
+Node.js native add-on. When an error occurs in your C++ code you have to handle
+and dispatch it correctly. **N-API** uses return values and JavaScript exceptions
+for error handling. You can choice one method or other based on your preferences


nit: "You can choose return values or exception handling based on the mechanism that works best for your add-on."

kfarnung · 2018-05-03T18:01:03Z

+
+The **Error** is a persistent reference (for more info see: [Object reference](object_reference.md)
+section) to a JavaScript error object. Use of this class depends somewhat on
+whether C++ exceptions are enabled at compile time.


nit: avoid "somewhat", we want to be as descriptive as possible. If there are more nuances we should try to enumerate them.

kfarnung · 2018-05-03T18:01:43Z

+The following sections explain the approach for each case:
+
+- [Handling Errors With C++ Exceptions](#exceptions)
+


nit: I don't think this extra newline is necessary here

kfarnung · 2018-05-03T18:02:18Z

+- [Handling Errors Without C++ Exceptions](#noexceptions)
+
+<a name="exceptions"></a>
+


nit: extra newline

I'm using markdownlint and it reports

[markdownlint] MD022/blanks-around-headers: Headers should be surrounded by blank lines

but if you want I can remove the extra space. Maybe in general we need to have a standard to follow.

kfarnung · 2018-05-03T18:04:07Z

-You are reading a draft of the next documentation and it's in continuos update so
-if you don't find what you need please refer to:
-[C++ wrapper classes for the ABI-stable C APIs for Node.js](https://nodejs.github.io/node-addon-api/)
+The **Error** class is a representation of the JavaScript Error object that is thrown


Do we have a guideline for these sorts of stylistic flairs? It seems like Error and Error are used in different scenarios throughout this file, what is the guideline we're following?

No at moment we haven't a stylistic standard to follow. My opinion is that we need one.

kfarnung · 2018-05-03T18:18:09Z

+
+Following C++ statements will not be executed. The exception will bubble up as a
+C++ exception of type **Error**, until it is either caught while still in C++, or
+else automatically propataged as a JavaScript exception when returns to


nit: "propagated"

nit: "C++ statements following the throwing statement will not be executed. The exception throw is of type Error and can be handled in C++ as usual. If the exception is not handled in C++ it will automatically propagate to the JavaScript caller.

kfarnung · 2018-05-03T18:23:33Z

+exception.
+
+<a name="noexceptions"></a>
+


nit: extra newline

Maybe an example that shows rethrowing ?

kfarnung · 2018-05-03T18:24:27Z

+## Handling Errors Without C++ Exceptions
+
+If C++ exceptions are disabled (for more info see: [Setup](setup.md) section),
+then the **Error**  class does not extend ```std::exception```. This means that


nit: extra space after "Error"

kfarnung · 2018-05-03T18:25:14Z

+
+If C++ exceptions are disabled (for more info see: [Setup](setup.md) section),
+then the **Error**  class does not extend ```std::exception```. This means that
+any call to node-addon-api functions does not throw C++ exception.


nit: "any calls to node-addon-api function do not throw C++ exceptions."

kfarnung

LGTM with some nits

mhdawson · 2018-05-03T21:23:38Z

+Initializes a ```Error``` instance from an existing ```Error``` object.
+
+```cpp
+TypeError(napi_env env, napi_value value);


I think this is a typo, should be Error instead of TypeError.

mhdawson · 2018-05-03T21:36:16Z

+Initializes a ```Error``` instance from an existing ```Error``` object.
+
+```cpp
+TypeError(napi_env env, napi_value value);


Believe this should Error not TypeError

mhdawson · 2018-05-03T21:38:06Z

+
+- [Handling Errors Without C++ Exceptions](#noexceptions)
+
+<a name="exceptions"></a>


? wondering why there is html here

This is to create an anchor with a friendly name that's not affected by changes to the name of the header. The same approach is used in the official CHANGELOG

ok, just have not see it in other docs.

mhdawson · 2018-05-03T21:41:38Z

+If C++ exceptions are enabled (for more info see: [Setup](setup.md) section),
+then the **Error** class extends ```std::exception``` and enables integrated
+error-handling for C++ exceptions and JavaScript exceptions.
+


If C++ exceptions are enabled then exceptions are converted and handled as C++ exceptions while in native code and as JavaScript exceptions in JavaScript code. What this means is that:

and then make the following bullet points:

mhdawson · 2018-05-03T21:42:04Z

+
+If a JavaScript function called by C++ code via N-API throws a JavaScript
+exception, then the N-API wrapper automatically converts and throws it as a C++
+exception of type **Error**.


on return from the JavaScript code to the native method.

mhdawson · 2018-05-03T21:42:57Z

+exception, then the N-API wrapper automatically converts and throws it as a C++
+exception of type **Error**.
+
+If a C++ exception of type **Error** escapes from a N-API C++ callback, then


On return from a native method, N-API will automatically convert a pending C++ exception to a JavaScript exception.

mhdawson · 2018-05-03T21:44:42Z

+If a C++ exception of type **Error** escapes from a N-API C++ callback, then
+the N-API wrapper automatically converts and throws it as a JavaScript exception.
+
+Therefore, catching a C++ exception of type **Error** prevents a JavaScript


When C++ exceptions are enabled try/catch can be used to catch exceptions thrown from calls to JavaScript and then they can either be handled or rethrown before returning from a native method.

mhdawson · 2018-05-03T21:49:24Z

+Napi::Function jsFunctionThatThrows = someObj.As<Napi::Function>();
+Napi::Value result = jsFunctionThatThrows({ arg1, arg2 });
+if (result.IsEmpty()) return;
+```


I think it would be better to suggest that they should check using IsExceptionPending as that will work for methods that don't return a value as well.

mhdawson · 2018-05-03T21:50:00Z

+```cpp
+Napi::Function jsFunctionThatThrows = someObj.As<Napi::Function>();
+Napi::Value result = jsFunctionThatThrows({ arg1, arg2 });
+if (result.IsEmpty()) {


Same goes here to check with IsExceptionPending

mhdawson · 2018-05-03T21:50:33Z

Overall looking good, just a few comments and suggestions.

NickNaso · 2018-05-10T12:49:47Z

Completed fix proposed on first review

mhdawson

LGTM

mhdawson · 2018-05-30T00:40:29Z

@NickNaso I'm having trouble landing. Can you squash the commits and then I'll try again.

First step of error documentation Documentation for error handling Complete fix proposed on first review

NickNaso · 2018-06-05T23:16:54Z

@mhdawson Could you try to land now?

NickNaso · 2018-06-12T00:12:05Z

Hi @mhdawson I combined the work on this PR with #272. Please don't land this PR and if you want close it.

mhdawson · 2018-06-14T14:48:36Z

Closing in favour of #272

NickNaso added 4 commits January 4, 2018 02:07

Update package.json example

825440a

Changed version of node-addon-api on the package.json example see: [issue 206](#206 (comment))

First step of error documentation

d11cd21

Start working on error documentation

178dd33

Documentation for error handling

794ee4d

NickNaso requested review from jschlight and mhdawson May 3, 2018 02:06

NickNaso mentioned this pull request May 3, 2018

Documentation push on node-add-api nodejs/abi-stable-node#280

Closed

kfarnung reviewed May 3, 2018

View reviewed changes

kfarnung approved these changes May 3, 2018

View reviewed changes

mhdawson reviewed May 3, 2018

View reviewed changes

mhdawson force-pushed the master branch from 12fe6f9 to a750ed1 Compare May 8, 2018 19:30

Complete fix proposed on first review

844b651

NickNaso removed the request for review from jschlight May 13, 2018 20:43

mhdawson approved these changes May 30, 2018

View reviewed changes

NickNaso added 2 commits June 6, 2018 01:03

Sqash the commits for error hadnling documentation

83fc181

First step of error documentation Documentation for error handling Complete fix proposed on first review

Merge remote-tracking branch 'origin/master'

ea2f927

mhdawson closed this Jun 14, 2018


		### Constructor

		Initializes a ```Error``` instance from an existing ```Error``` object.

		The following sections explain the approach for each case:

		- [Handling Errors With C++ Exceptions](#exceptions)

		- [Handling Errors Without C++ Exceptions](#noexceptions)

		<a name="exceptions"></a>

Uh oh!

Conversation

NickNaso commented May 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kfarnung left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhdawson commented May 3, 2018