Include more information in error message when fetch() fails #647
Conversation
|
No, there wasn't an intention to keep the message short, so this is a welcome improvement, thanks. |
| @@ -116,7 +117,7 @@ func (t *fetchTransport) RoundTrip(req *Request) (*Response, error) { | |||
| }, | |||
| func(reason *js.Object) { | |||
| select { | |||
| case errCh <- errors.New("net/http: fetch() failed"): | |||
| case errCh <- fmt.Errorf("net/http: fetch() failed: %s", reason.String()): | |||
There was a problem hiding this comment.
Can you be sure that reason will never be nil? Otherwise it'd be a nil pointer dereference panic, right?
There was a problem hiding this comment.
I guess it might never be nil for the same reason result is never nil in the success case.
I looked at https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then, and don't see an explicit confirmation that reason won't be nil...
Is there a way we can be sure?
There was a problem hiding this comment.
Done. I'm not 100% sure and it's pretty easy to add this check.
There was a problem hiding this comment.
It'd be nice if we could find out. That way, the code could be simpler.
| @@ -115,8 +116,15 @@ func (t *fetchTransport) RoundTrip(req *Request) (*Response, error) { | |||
| } | |||
| }, | |||
| func(reason *js.Object) { | |||
| var jsErr error | |||
There was a problem hiding this comment.
Style-wise, just name this err. I don't see why it should be jsErr.
| if reason != nil { | ||
| jsErr = fmt.Errorf("net/http: fetch failed: %s", reason.String()) | ||
| } else { | ||
| jsErr = errors.New("net/http: fetch() failed") |
There was a problem hiding this comment.
Why does one message have "fetch" but the other "fetch()"? It'd be better to make them consistent.
|
I found that it's indeed possible to pass var p1 = new Promise( (resolve, reject) => {
//resolve('Success!');
// or
reject (null);
} );
p1.then( value => {
console.log(value); // Success!
}, reason => {
console.log(reason === null); // true
console.log(reason); // "null"
} );(On JSFiddle.) That prints So the question becomes, does any implementation of Fetch API ever pass |
|
I addressed the style comments. As far as I can tell, neither the ECMAScript 2015 Spec for Promises nor the Living Standard for the Fetch API explicitly forbid a null value showing up here. A quick test shows that Chrome allows you to do things like this: Promise.reject(null).catch(function(e) {console.log(e)})
Promise.reject(undefined).catch(function(e) {console.log(e)})I'm not sure what the best course of action is, so I'll defer to you or anyone else who can chime in. |
|
Ah, well, I just remembered one GopherJS-specific thing. It's actually safe to call the E.g., this is fine: https://gopherjs.github.io/playground/#/kY9ne7yWeM It's effectively equivalent to: func (o *Object) String() string {
if o == nil {
return "null"
}
return string(*o) // not really, but you get the idea
}So, no panic is possible. You can revert to your original code, since it's shorter and better. That said, we might want to improve the documentation of |
6a451b8 to
c7fd67f
Compare
|
Cool. I reverted back to the original state of this PR (without the nil check). |
I recently ran into a frustrating issue when running browser tests for go-humble/rest. The only error message I was seeing was:
After I made a small tweak to GopherJS, the error message turns into:
With this additional information I was able to quickly fix the bug in go-humble/rest and move on.
I can understand if you prefer to keep the error message short, but it would have saved me a lot of time to have this longer error message available from the beginning. I suspect this will save other people some time too.