pmed / v8pp

Bind C++ functions and classes into V8 JavaScript engine
http://pmed.github.io/v8pp/
Other
901 stars 121 forks source link

v8pp throws strings by default, which is bad practice #205

Open jcmonnin opened 9 months ago

jcmonnin commented 9 months ago

v8pp::throw_ex throws a string by default, which is considered a bad practice by many:

https://stackoverflow.com/questions/11502052/throwing-strings-instead-of-errors

In the application I'm working on, errors from user scripts are formatted in a standard way including stack traces, but it fails if the error is thrown within v8pp. The user is left without stack trace. For that reason I have a patch to default to throwing and Error instead of a string in v8pp.

Would this patch be welcome upstream?

pmed commented 9 months ago

Hi Jean-Claude,

You have proposed the right thing, thanks 👍. A possible fix would be just using exception_ctor = v8::Exception::Error default parameter value for throw_ex(), isn't it?

Sure, PR is welcomed!

jcmonnin commented 9 months ago

I pushed a PR.

In my own binding code I use following helper functions (which originates from the time where v8pp didn't even have the option to specify an exception type).

inline void throwRangeError(v8::Isolate* isolate, std::string_view message)
{
    isolate->ThrowException(v8::Exception::RangeError(v8pp::to_v8(isolate, message)));
}

inline void throwReferenceError(v8::Isolate* isolate, std::string_view message)
{
    isolate->ThrowException(v8::Exception::ReferenceError(v8pp::to_v8(isolate, message)));
}

inline void throwSyntaxError(v8::Isolate* isolate, std::string_view message)
{
    isolate->ThrowException(v8::Exception::SyntaxError(v8pp::to_v8(isolate, message)));
}

inline void throwTypeError(v8::Isolate* isolate, std::string_view message)
{
    isolate->ThrowException(v8::Exception::TypeError(v8pp::to_v8(isolate, message)));
}

inline void throwError(v8::Isolate* isolate, std::string_view message)
{
    isolate->ThrowException(v8::Exception::Error(v8pp::to_v8(isolate, message)));
}

Given there are only a handful of exception types, a similar API which could be considered. The current v8pp::throw_ex would be an implementation detail and there would be v8pp::throw_error, v8pp::throw_range_error, ,..

Admittedly, the current API with the ctor function pointer is fine too.

pmed commented 9 months ago

I like your idea, to have a set of functions for throwing particular exception types. Do you mean to have them one-liners that call throw_ex() like:

inline v8::Local<v8::Value> throw_error(v8::Isolate* isolate, std::string_view message, v8::Local<v8::Value> exception_options = {})
{
    return throw_ex(isolate, message, v8::Exception::Error, exception_options);
}

inline v8::Local<v8::Value> throw_type_error(v8::Isolate* isolate, std::string_view message, v8::Local<v8::Value> exception_options = {})
{
    return throw_ex(isolate, message, v8::Exception::TypeError, exception_options);
}
jcmonnin commented 8 months ago

Yes, exactly.