[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.


---

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

[jeking3]

Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1075
  
@bananer is this worth pulling forward if it passes a build?  Is it a 
breaking change?


---

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

[jeking3]

Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1075
  
@SimenB if you are interested in resurrecting this let me know - rebase on 
current master and push to CI will run all our tests.


---

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

[jeking3]

Github user jeking3 commented on the issue:

https://github.com/apache/thrift/pull/1075
  
@SimenB if you are interested in resurrecting this let me know - rebase on 
current master and push to CI will run all our tests.


---

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

[nsuke]

Github user nsuke commented on the issue:

https://github.com/apache/thrift/pull/1075
  
@SimenB we have `Dokerfile` s for build env inside build/docker/ 
subdirectories.
You can also do `docker pull thrift/ubuntu` or thrift/debian.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---