[ 
https://issues.apache.org/jira/browse/THRIFT-3916?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16397108#comment-16397108
 ] 

ASF GitHub Bot commented on THRIFT-3916:
----------------------------------------

Github user bananer commented on the issue:

    https://github.com/apache/thrift/pull/1075
  
    @jeking3 This is a well-intended change but I think it is not implemented 
correctly and will break error handling for users.
    
    In some places in the compiler the input to `render_recv_throw` already is 
a proper `Error` (the JS exception class), e.g. 
https://github.com/apache/thrift/blob/2b09dfed9c6b858571e7d8829a2b4a4bcda18d6a/compiler/cpp/src/thrift/generate/t_js_generator.cc#L1648-L1652
    
    This is not the case for other places:
    
https://github.com/apache/thrift/blob/2b09dfed9c6b858571e7d8829a2b4a4bcda18d6a/compiler/cpp/src/thrift/generate/t_js_generator.cc#L1673
    
    The proper way of doing this would be to inspect all usages of 
`render_recv_throw` to see if the argument needs to be wrapped in `new 
Error(…)`. Still then, this could break error handling that relies on these 
cases being strings.


> Errors thrown from JavaScript client is strings and not errors
> --------------------------------------------------------------
>
>                 Key: THRIFT-3916
>                 URL: https://issues.apache.org/jira/browse/THRIFT-3916
>             Project: Thrift
>          Issue Type: Bug
>          Components: JavaScript - Compiler
>            Reporter: Simen Bekkhus
>            Priority: Minor
>              Labels: javascript
>
> In JavaScript, you can {{throw}} any object, including strings, but to get 
> stack traces one should throw {{Errors}}.
> The generated JS code throws the error string directly, instead of wrapping 
> it in {{new Error}}.
> Node core _only_ rejects with/throws Errors, never strings, and Thrift should 
> follow the same standard.
> https://nodejs.org/api/errors.html#errors_class_error
> {quote}
> All errors generated by Node.js, including all System and JavaScript errors, 
> will either be instances of, or inherit from, the Error class.
> {quote}
> PR for the change [here|https://github.com/apache/thrift/pull/1075].



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to