[GitHub] thrift issue #1075: THRIFT-3916 Throw proper errors from JS, not strings

[bananer]

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.


---