Documentation for error handling #259

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

doc/error.md

+
+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

doc/error.md

+### 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

doc/error.md

+```
+
+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

doc/error.md

+
+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

doc/error_handling.md

@@ -1,5 +1,138 @@
 # Error handling

-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

doc/error_handling.md

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

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

doc/error_handling.md

+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

doc/error_handling.md

+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

doc/error_handling.md

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


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

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

doc/error_handling.md

+
+<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

doc/error.md

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

doc/error_handling.md

+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

doc/error_handling.md

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


nit: extra newline

Maybe an example that shows rethrowing ?

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

doc/error_handling.md

+
+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

doc/error_handling.md

+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

doc/error.md

+
+```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

doc/error.md

+
+```cpp
+TypeError(napi_env env, napi_value value);


Believe this should Error not TypeError

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

doc/error_handling.md

+- [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

doc/error_handling.md

+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

doc/error_handling.md

+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

doc/error_handling.md

+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

doc/error_handling.md

+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

doc/error_handling.md

+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

doc/error_handling.md

+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 mhdawson and jschlight 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


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

		### Constructor

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


		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.


		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

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

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

		then the Error class extends ```std::exception``` and enables integrated
		error-handling for C++ exceptions and JavaScript exceptions.

		exception of type Error.

		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

Documentation for error handling #259

Documentation for error handling #259

Conversation

NickNaso commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhdawson commented May 3, 2018

NickNaso commented May 10, 2018

mhdawson left a comment

Choose a reason for hiding this comment

mhdawson commented May 30, 2018

NickNaso commented Jun 5, 2018

NickNaso commented Jun 12, 2018

mhdawson commented Jun 14, 2018