-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[file_packager] Convert openDatabase to async/await. NFC #24883
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Best viewed with "hide whitespace". |
I hope you don't mind me splitting this up into small peices like this. Its easier for me to get my head around each step on its own and I hope its easier to review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm % question
+1 for the splitting up!
tools/file_packager.py
Outdated
code += ''' | ||
if (isNode) { | ||
return errback(); | ||
}''' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like node always errored. Is this fixing that path?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(do we test it?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the typeof indexedDB == 'undefined'
just below should be enough to handle the node case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. maybe I should split that out, or at least see why it was added.
This was added back in emscripten-core#22802 but seems redundant since the check just below for the indexedDB global should be enough to handle the node case. Split out from emscripten-core#24883
This was added back in emscripten-core#22802 but seems redundant since the check just below for the indexedDB global should be enough to handle the node case. Split out from emscripten-core#24883
fd6035c
to
c232a88
Compare
c232a88
to
466604a
Compare
@@ -805,34 +805,32 @@ def generate_js(data_target, data_files, metadata): | |||
var DB_VERSION = 1; | |||
var METADATA_STORE_NAME = 'METADATA'; | |||
var PACKAGE_STORE_NAME = 'PACKAGES'; | |||
function openDatabase(callback, errback) { | |||
async function openDatabase() { | |||
if (typeof indexedDB == 'undefined') { | |||
return errback('using IndexedDB to cache data can only be done on a web page or in a web worker'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point! Will fix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this went unnoticed because the errback is not defined
error was caught an the fallback was run anyway:
$ node ./src.js
ReferenceError: errback is not defined
at openDatabase (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:173:13)
at runWithFS (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:333:9)
at callRuntimeCallbacks (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:1164:26)
at preRun (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:841:3)
at run (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:4658:3)
at Object.removeRunDependency (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:950:7)
at loadPackage (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:362:34)
at runMetaWithFS (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:371:18)
falling back to default preload behavior
done
Still worth fixing though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in #24886
This went unnoticed by the tests because the calling code has a `.catch` handler that calls the fallback function. So the test still passed but with a bunch of extra output: ``` ReferenceError: errback is not defined at openDatabase (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:173:13) at runWithFS (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:333:9) at callRuntimeCallbacks (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:1164:26) at preRun (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:841:3) at run (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:4658:3) at Object.removeRunDependency (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:950:7) at loadPackage (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:362:34) at runMetaWithFS (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:371:18) falling back to default preload behavior done ``` Followup to emscripten-core#24883
This went unnoticed by the tests because the calling code has a `.catch` handler that calls the fallback function. So the test still passed but with a bunch of extra output: ``` ReferenceError: errback is not defined at openDatabase (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:173:13) at runWithFS (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:333:9) at callRuntimeCallbacks (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:1164:26) at preRun (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:841:3) at run (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:4658:3) at Object.removeRunDependency (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:950:7) at loadPackage (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:362:34) at runMetaWithFS (/usr/local/google/home/sbc/dev/wasm/emscripten/out/test/src.js:371:18) falling back to default preload behavior done ``` Followup to #24883
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Part 4 in the sequence following emscripten-core#24882, emscripten-core#24883 and emscripten-core#24885.
Followup to #24882