Add error event hander for the body stream even if the body isn't accessed #379
Conversation
Codecov Report
@@ Coverage Diff @@
## master #379 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 6 6
Lines 424 431 +7
Branches 133 135 +2
=====================================
+ Hits 424 431 +7
Continue to review full report at Codecov.
|
|
I think this make sense but I am not certain how browser actually handle this. Fetch Spec seems to suggest a different approach (see step 10): |
|
I tested this in firefox and chrome and both seem to behave like I implemented it. |
|
@MoritzKn Sorry I haven't had time to look deep into browser behavior recently. Do you happen to have any screenshots or output from browsers? Do they actually reject invalid encoding instead of just returning result without decompression or garbled result? The last time I checked Fetch doesn't expose this sort of information. |
|
Sorry I have no screenshots of the output or anything but I could make some this evening. In my tests with invalid encoding the fetch promise was resolve but the |
|
@bitinn little later then promised, sorry. Running this code in node: const http = require('http');
const url = require('url');
const server = http.createServer((req, res) => {
if (url.parse(req.url).pathname === '/invalid') {
res.setHeader('Content-Encoding', 'gzip');
res.end('not gzip');
return;
}
res.setHeader('Content-Type', 'text/html; charset=utf-8');
res.end(`
<!doctype html>
<title>Test</title>
<script>
fetch('http://localhost:12345/invalid')
.then(result => console.log('simple request is fine', result))
.catch(error => console.error('error with simple request', error))
fetch('http://localhost:12345/invalid').then(res => res.text())
.then(result => console.log('calling .text() is fine', result))
.catch(error => console.error('error with calling .text()', error))
fetch('http://localhost:12345/invalid').then(res => res.blob())
.then(result => console.log('calling .blob() is fine', result))
.catch(error => console.error('error with calling .blob()', error))
</script>
`);
});
server.listen(12345, 'localhost', () => {
console.log('listening at http://localhost:12345');
});and opening http://localhost:12345/ in Firefox show this in the console: and this in Chrome: |
|
The github polyfill however rejects the error directly: I applied the polyfill without the check if If I have time this weekend, I will try to look into this further and perhaps file a spec issue. The behavior doesn't seem well defined. |
|
Accidentally closed |
|
Thx for looking into this and producing a concise test case. My guess is browsers doesn't consider this an important case to normalize because there will be no break-the-world exceptions. |
|
Without looking at the current PR, from a spec standpoint this is pretty well defined. Other than parameter validation errors, |
|
@TimothyGu indeed that is what the PR does. I got around to test this in Safari and Edge too. Running the above snippet and opening the page in Safari results in these logs: It logs that something went wrong (Failed to load resource: decoding the raw data not possible.) in the console but resolves the promise. In Edge the behavior is similar to Chrome and Firefox however the Nevertheless I think an uncaught exception that can't be handled and causes the node process to crash isn't desirable in any case. |
|
I now also created an issue on the fetch spec: whatwg/fetch#657 |
Fixes #378