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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 3 commits into from
Closed

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

wants to merge 3 commits into from

Conversation

joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Jan 14, 2018 •
edited
Loading

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

@joyeecheung
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 nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 14, 2018
targos
targos approved these changes Jan 14, 2018
View reviewed changes
CPP_STYLE_GUIDE.md Outdated

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
Copy link
Member

Choose a reason for hiding this comment

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

should be done

Copy link
Member Author

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!

vsemozhetbyt
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

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
vsemozhetbyt reviewed Jan 14, 2018
View reviewed changes
CPP_STYLE_GUIDE.md Outdated
const ctx = {};
const result = binding.foo(str, ctx);
if (ctx.code !== undefined) {
throw errors.Error('ERR_ERROR_NAME', ctx.code);
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing new?

jasnell
jasnell reviewed Jan 14, 2018
View reviewed changes
CPP_STYLE_GUIDE.md Outdated
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++.
Copy link
Member

Choose a reason for hiding this comment

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

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

@joyeecheung
Copy link
Member Author

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

@joyeecheung 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
mhdawson
mhdawson approved these changes Jan 15, 2018
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

TimothyGu
TimothyGu reviewed Jan 16, 2018
View reviewed changes
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.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Member

Choose a reason for hiding this comment

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

Either is fine.

Copy link
Member Author

Choose a reason for hiding this comment

The 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 Maybes. We also need a section about using the Maybe APIs instead of the deprecated V8 APIs.

@joyeecheung
Copy link
Member Author

Landed in 27925c4, thanks!

joyeecheung added a commit that referenced this pull request Jan 16, 2018
@joyeecheung
doc: suggest not to throw JS errors from C++
27925c4 
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>
@joyeecheung 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 pushed a commit that referenced this pull request Jan 30, 2018
@joyeecheung @evanlucas
doc: suggest not to throw JS errors from C++
b05f09a 
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>
@evanlucas evanlucas mentioned this pull request Jan 30, 2018
Merged
MylesBorins pushed a commit that referenced this pull request Feb 27, 2018
@joyeecheung @MylesBorins
doc: suggest not to throw JS errors from C++
afc5289 
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@TimothyGu TimothyGu TimothyGu left review comments

@starkwang starkwang starkwang approved these changes

@mhdawson mhdawson mhdawson approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@danbev danbev danbev approved these changes

Assignees
No one assigned
Labels
doc Issues and PRs related to the documentations.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.