Test BigInt as keys and values in IndexedDB #9667
Conversation
BigInt and BigInt wrappers are supported in serialization, per whatwg/html#3480 This support allows them to be used as IndexedDB values. However, BigInt is not supported as an IndexedDB key; support has been proposed in the following PR, but that change has not landed at the time this patch was written w3c/IndexedDB#231
wpt-pr-bot
requested review from
chunywang,
dumbmatter,
odinho,
yunxiaoxie,
zhaozihao and
zqzhang
| "condition does not apply to initial value"); | ||
| }); | ||
|
|
||
| createdb(t).onupgradeneeded = function(e) { |
There was a problem hiding this comment.
wrap the function in t.step_func() so that unexpected exceptions thrown in the async callback are tied to the test, e.g.
createdb(t).onupgradeneeded = t.step_func(e => { ... });|
|
||
| let i = 0; | ||
| function value(value, test) { | ||
| var t = async_test(document.title + " - " + (i++)); |
There was a problem hiding this comment.
style nit: I find the async_test(t => { /*test body... */ }, description); form more readable, and it appears to be more common.
There was a problem hiding this comment.
It would be great if the test name said something about what was being tested in this case; rather than document.title, how about "BigInts as values in IndexedDB"
This really helps implementers/reviewers when looking a big table of failures: you can tell what the failure was without reading the test.
Can this be passed in to the value() function rather than just using numbers, so it's clear what each case does?
|
|
||
| </script> | ||
|
|
||
| <div id="log"></div> |
There was a problem hiding this comment.
not needed; testharness will create this dynamically as needed
| // https://github.com/whatwg/html/pull/3480 | ||
| // This support allows them to be used as IndexedDB values. | ||
|
|
||
| let i = 0; |
There was a problem hiding this comment.
Maybe call this unique_test_id to make the usage clear?
| @@ -0,0 +1,65 @@ | |||
| <!DOCTYPE html> | |||
| <meta charset="utf-8"> | |||
| <title>Values</title> | |||
There was a problem hiding this comment.
Maybe clarify the name: BigInts as Values and Keys in IndexedDB ?
| }; | ||
| } | ||
|
|
||
| value(1n, x => x === 1n); |
There was a problem hiding this comment.
Add case where it's not the top-level record, e.g. {val: 1n} ?
| // https://github.com/w3c/IndexedDB/pull/231 | ||
|
|
||
| function invalidKey(key) { | ||
| var t = async_test(document.title + " - " + (i++)); |
There was a problem hiding this comment.
Same comments as above, re: style and description.
| function invalidKey(key) { | ||
| var t = async_test(document.title + " - " + (i++)); | ||
|
|
||
| createdb(t).onupgradeneeded = function(e) { |
There was a problem hiding this comment.
t.step_func is needed here too.
|
Thanks for this detailed review. I apologize for my delay in uploading a fixed-up PR. I intend to do so within 2-3 weeks; I'm just a bit busy with TC39 happening. |
|
Obsoleted by #10977 |
BigInt and BigInt wrappers are supported in serialization, per
whatwg/html#3480
This support allows them to be used as IndexedDB values.
However, BigInt is not supported as an IndexedDB key; support
has been proposed in the following PR, but that change has not
landed at the time this patch was written
w3c/IndexedDB#231