[file_packager] Convert fetchRemotePackage to async. NFC #24893
Conversation
sbc100
changed the title
sbc100
force-pushed
the
file_packager_async_part4
branch
4 times, most recently
from
e5efcc8 to
13305e0
Compare
sbc100
force-pushed
the
file_packager_async_part4
branch
from
13305e0 to
f874811
Compare
tools/file_packager.py
Outdated
| } | ||
| var response = await fetch(packageName); | ||
| if (!response.ok) { | ||
| throw `${response.status}: ${response.url}`; |
There was a problem hiding this comment.
Please wrap it back into new Errror.
Unfortunately, Emscripten codebase already suffers from many legacy codepaths that just throw strings, but it's something I'm hoping we'll fix over time, not introduce more.
It's a subtle difference for those who are not familiar with it, but while JS supports throwing arbitrary primitives from its early days, anything but descendants of Error results in an error with no stacktrace and no way to find out where the error occurred or why, which makes the error messages almost useless.
There was a problem hiding this comment.
Another option is probably to just use abort(...) here which does the right thing already.
There was a problem hiding this comment.
abort() might be best, yeah.
Otherwise, I think in a release build it can make sense to throw a raw string? We don't care about the stack trace/debuggability there, and it is smaller.
There was a problem hiding this comment.
I don't think we can use abort here because this code doesn't always run inside of emscripten-generated code.. it can also run before the emscripten code loads.
| const iterate = () => reader.read().then(handleChunk).catch((cause) => { | ||
| return Promise.reject(new Error(`Unexpected error while handling : ${response.url} ${cause}`, {cause})); | ||
| }); | ||
| const chunks = []; |
There was a problem hiding this comment.
Anything here and on can be removed nowadays - all browsers we care about support response.arrayBuffer() so it's enough to return just that.
There was a problem hiding this comment.
Ah, or is the goal to show actual progress? Then perhaps the opposite, remove the polyfill.
There was a problem hiding this comment.
yes, the goal is to show progress.
I don't know which polyfill is being referred to above, but its not something emscripten includes.
Maybe it can be remove now? It was added in #22016 but I don't see anything about if/when the polyfill might be used. Probably best to leave that to a separate cleanup PR.
sbc100
force-pushed
the
file_packager_async_part4
branch
from
f874811 to
6d48d48
Compare
This argument has been used since 3839dd4. Even when this function was first add in 74f3242 the errback callback was only used for the IDB failures, not the fetch failures. Fetch failure were left to propagate to the global error handlers. This is small cleanup in advance of emscripten-core#24893.
This argument has been used since 3839dd4. Even when this function was first add in 74f3242 the errback callback was only used for the IDB failures, not the fetch failures. Fetch failure were left to propagate to the global error handlers. This is small cleanup in advance of emscripten-core#24893.
This argument has basically used since 3839dd4. Even when this function was first add in 74f3242 the errback callback was only used for the IDB failures, not the fetch failures. Fetch failure were left to propagate to the global error handlers. The exception here was that when node support was added in emscripten-core#11785 it, I believe incorrectly used errback to handle file read errors (doesn't match the web version that does *not* handle fetch error using errback). This is small cleanup in advance of emscripten-core#24893.
This argument has been used since 3839dd4. Even when this function was first added in 74f3242, `errback` was only used for the IDB failures, not the fetch failures. Fetch failure were left to propagate to the global error handlers. The exception here was that when node support was added in #11785 it, I believe incorrectly used errback to handle file read errors (doesn't match the web version that does *not* handle fetch error using errback). I'm not claiming this is correct/best behaviour, this is simply an NFC to cleanup an already-unused argument. This is small cleanup in advance of #24893.
The last usage was removed in emscripten-core#24904 but I forgot to remove the function itself.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
sbc100
force-pushed
the
file_packager_async_part4
branch
from
6d48d48 to
88f4523
Compare
Part 4 in the sequence following #24882, #24883 and #24885.