Skip to content
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

Test BigInt as keys and values in IndexedDB #10977

Merged
merged 2 commits into from
May 22, 2018

Conversation

cxielarko
Copy link

@cxielarko cxielarko commented May 13, 2018

Test BigInt serialization in IndexedDB values, and check that BigInt values cannot be used as IndexedDB keys. These are @littledan's tests with some changes suggested in reviews; see PR #9667 for the original version.

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
@cxielarko
Copy link
Author

cc @littledan @inexorabletash

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the submission!


function invalidKey(key, name) {
async_test(t => {
createdb(t).onupgradeneeded = t.step_func(e => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easier to just use e.g. indexedDB.cmp(0, key) - this will throw if the key is not valid. No need to create a database.

async_test(t => {
t.step(function() {
assert_true(test(value),
"condition does not apply to initial value");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assertion messages should be phrased in the form of the expected behavior, e.g. "Predicate should return true for the initial value"; the error output is confusing if the inverse is used.

// This support allows them to be used as IndexedDB values.

let i = 0;
function value(value, test, name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rename the test argument to test_func or predicate? Since test is a testharness function it's confusing to see it re-used as an argument.

// This support allows them to be used as IndexedDB values.

let i = 0;
function value(value, test, name) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using value as both the name of the function and the argument is confusing. How about value_test for the function?

.createObjectStore("store")
.add(value, 1);

e.target.onsuccess = t.step_func(function(e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow function instead of function ?

.transaction("store")
.objectStore("store")
.get(1)
.onsuccess = t.step_func(function(e)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Arrow function instead of function ?

@@ -0,0 +1,77 @@
<!DOCTYPE html>
<meta charset="utf-8">
<title>Values</title>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Provide a more informative title

<script src="support.js"></script>

<script>
// BigInt and BigInt wrappers are supported in serialization, per
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "objects" instead of "wrappers" to be consistent with the usage below? Or use "wrappers" everywhere.

// https://github.com/whatwg/html/pull/3480
// This support allows them to be used as IndexedDB values.

let i = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unused?

@cxielarko
Copy link
Author

implemented the requested changes, and added a missing .valueOf() call

Copy link
Member

@inexorabletash inexorabletash left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

@inexorabletash inexorabletash merged commit b2e3e49 into web-platform-tests:master May 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants