doc: suggest not to throw JS errors from C++ by joyeecheung · Pull Request #18149 · nodejs/node

joyeecheung · 2018-01-14T18:56:40Z

Also provide an example on how to use internal/errors
to handle errors in C++.

Refs: #18106

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)

doc

Also provide an example on how to use internal/errors to handle errors in C++.

targos · 2018-01-14T19:03:15Z

CPP_STYLE_GUIDE.md


-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


should be done

@targos Fixed, thanks for catching that!

vsemozhetbyt · 2018-01-14T20:34:36Z

CPP_STYLE_GUIDE.md

+    throw errors.Error('ERR_ERROR_NAME', ctx.code);
+  }
+  return result;
+}


A nit: missing semicolon.

vsemozhetbyt · 2018-01-14T20:37:39Z

CPP_STYLE_GUIDE.md

+  const ctx = {};
+  const result = binding.foo(str, ctx);
+  if (ctx.code !== undefined) {
+    throw errors.Error('ERR_ERROR_NAME', ctx.code);


Missing new?

jasnell · 2018-01-14T21:22:20Z

CPP_STYLE_GUIDE.md

-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 be done in JavaScript
+before the arguments are passed into C++.


Perhaps add something about including a CHECK at the C++ level?

joyeecheung · 2018-01-15T15:51:22Z

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite/86/

mhdawson

LGTM

TimothyGu · 2018-01-16T06:43:58Z

CPP_STYLE_GUIDE.md

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


"Maybe" mention Maybe<> and MaybeLocal<> as possible methods to pass exception status if exceptions have to be transported through nested C++ calls.

Hmm I think we could make a separate section for the Maybes and link from here?

Either is fine.

@TimothyGu Since this was how it's documented prior to this PR, I'll open a separate PR for the usage of Maybes. We also need a section about using the Maybe APIs instead of the deprecated V8 APIs.

joyeecheung · 2018-01-16T18:43:20Z

Landed in 27925c4, thanks!

Also provide an example on how to use internal/errors to handle errors in C++. PR-URL: #18149 Refs: #18106 Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

doc: suggest not to throw JS errors from C++

ccfd2b1

Also provide an example on how to use internal/errors to handle errors in C++.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 14, 2018

targos approved these changes Jan 14, 2018

View reviewed changes

[squash] fix typo

877afe1

maclover7 approved these changes Jan 14, 2018

View reviewed changes

vsemozhetbyt reviewed Jan 14, 2018

View reviewed changes

CPP_STYLE_GUIDE.md Outdated

throw errors.Error('ERR_ERROR_NAME', ctx.code);

}

return result;

}

Copy link

Contributor

vsemozhetbyt Jan 14, 2018

Choose a reason for hiding this comment

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

A nit: missing semicolon.

vsemozhetbyt reviewed Jan 14, 2018

View reviewed changes

jasnell reviewed Jan 14, 2018

View reviewed changes

starkwang approved these changes Jan 15, 2018

View reviewed changes

danbev approved these changes Jan 15, 2018

View reviewed changes

[squash] mention C++ CHECKs, fix examples

d42a2b5

jasnell approved these changes Jan 15, 2018

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 15, 2018

cjihrig approved these changes Jan 15, 2018

View reviewed changes

mhdawson approved these changes Jan 15, 2018

View reviewed changes

TimothyGu reviewed Jan 16, 2018

View reviewed changes

joyeecheung closed this Jan 16, 2018

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 16, 2018

evanlucas mentioned this pull request Jan 30, 2018

v9.5.0 release proposal #18464

Merged

MylesBorins added the dont-land-on-v6.x label Feb 27, 2018

MylesBorins added the land-on-v8.x label Feb 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

doc: suggest not to throw JS errors from C++#18149

doc: suggest not to throw JS errors from C++#18149
joyeecheung wants to merge 3 commits intonodejs:masterfrom
joyeecheung:js-errors-guide

joyeecheung commented Jan 14, 2018 •

edited

Loading

Uh oh!

targos Jan 14, 2018

Uh oh!

joyeecheung Jan 14, 2018

Uh oh!

vsemozhetbyt Jan 14, 2018

Uh oh!

vsemozhetbyt Jan 14, 2018

Uh oh!

jasnell Jan 14, 2018

Uh oh!

joyeecheung commented Jan 15, 2018

Uh oh!

mhdawson left a comment

Uh oh!

TimothyGu Jan 16, 2018

Uh oh!

joyeecheung Jan 16, 2018

Uh oh!

TimothyGu Jan 16, 2018

Uh oh!

joyeecheung Jan 16, 2018

Uh oh!

joyeecheung commented Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

Uh oh!

Conversation

joyeecheung commented Jan 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

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!

joyeecheung commented Jan 15, 2018

Uh oh!

mhdawson 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!

joyeecheung commented Jan 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

joyeecheung commented Jan 14, 2018 •

edited

Loading