Conversation
|
|
||
| Local<Value> result = script.ToLocalChecked()->Run(); | ||
| if (result.IsEmpty()) { | ||
| ReportException(env, try_catch); |
There was a problem hiding this comment.
What did the try_catch do? If it's not needed any longer, remove it from line 5259.
There was a problem hiding this comment.
To be honest with you, i dont know exactly what the TryCatch does. However, it is also used in line 5267 (if (script.IsEmpty())). Therefore, i would not remove it entirely (at least not until we figured out more about it).
| exit(4); //TODO jh: don't exit process when function breaks. Handle error differently. | ||
| } | ||
|
|
||
| return scope.Escape(result); |
There was a problem hiding this comment.
scope.Escape(...) pushes the given handle into the previously active scope.
This is used in order to save the result, once the script returned (since the scripts scope should be closed after it was executed).
It seems that this is needed for our usecase as well, therefore i will put it back in the code within my next commit.
|
|
||
| if (!maybe_arg.ToLocal(&arg)) { | ||
| // cannot create v8 string | ||
| return MaybeLocal<v8::Object>(); |
There was a problem hiding this comment.
Why don't we return maybe_arg here? Couldn't it carry valuable information for the application programmer?
There was a problem hiding this comment.
generally, why do we always return a new MaybeLocal object and not the one that failed?
There was a problem hiding this comment.
maybe_arg is of type MaybeLocal<v8::String> while the method returns MaybeLocal<v8::Object>. Casting one MaybeLocal<T> to another MaybeLocal<V> is only possible when casting to a Local<T> and then to Local<V>, which then can be wrapped inside a MaybeLocal<V>.
Long story short: Its not the way we want the code to look like. trust me
cmfcmf
left a comment
cmfcmf
left a comment
There was a problem hiding this comment.
👍 Nicely done. I've only got one more minor thing (see above).
Do you mind opening an issue to investigate what try_catch does? It might not only apply to the particular function but could also potentially be used in our other functions as well.
Nevermind, looks all good 👍 |
|
We just fixed the apps to actually work with these changes, shame on the approvers of this PR!! 😉 |
4 participants
No description provided.