Level / level-js

An abstract-leveldown compliant store on top of IndexedDB.
MIT License
544 stars 44 forks source link

Test all key types #128

Closed vweevers closed 6 years ago

vweevers commented 6 years ago

https://github.com/Level/level-js/blob/c471684f704586cdb7201ec859f774fa281255ae/index.js#L114-L122

vweevers commented 6 years ago

Hmm, keys and values behave differently atm. Invalid keys (according to the spec, like NaN) will be stringified, but invalid values (like Function) won't and cause the browser to throw an error.

I think invalid keys should also throw, to avoid surprises. We'd still stringify valid keys, to provide reasonable fallbacks. E.g. if the browser doesn't support binary keys, we convert them to strings.

@ralphtheninja WDYT?

vweevers commented 6 years ago

Essentially I mean to remove the String(key) at the end of:

https://github.com/Level/level-js/blob/b73567846177832f79210415f85295c9a7af0068/index.js#L124-L134

Which also means we can add support of ArrayBuffer and TypedArray, without type checking. And we don't need the if (typeof key === 'number' || isDate(key)) either!

ralphtheninja commented 6 years ago

Hmm, keys and values behave differently atm. Invalid keys (according to the spec, like NaN) will be stringified, but invalid values (like Function) won't and cause the browser to throw an error.

I think invalid keys should also throw, to avoid surprises. We'd still stringify valid keys, to provide reasonable fallbacks. E.g. if the browser doesn't support binary keys, we convert them to strings.

@ralphtheninja WDYT?

I agree. It makes it more clear what is valid and also consistent behavior.

vweevers commented 6 years ago

Sadly it breaks the abstract test suite, which tests boolean keys (among other things).

vweevers commented 6 years ago

And it also tests NaN :(

ralphtheninja commented 6 years ago

Lets leave it hanging for a while and let the thought mature. We can also counter it with good docs.

vweevers commented 6 years ago

It can be fixed ad-hoc with a if (typeof key === 'boolean' || isNaN(key)) return String(key).

vweevers commented 6 years ago

Damn! Just found out that Chrome returns binary keys as strings. The Iterator entries look like { key: '0,127', value: 'foo' }. Though get() etc does work.

I entirely missed this. Apparently IndexedDB internally converts keys to a key construct with an "associated type which is one of number, date, string, binary, or array". And with an "associated value, which will be either an unrestricted double if type is number or date, a DOMString if type is string, a list of octets if type is binary, or a list of other keys if type is array."

So for ArrayBuffer or TypedArray, the process for converting a key is:

If input is a buffer source type

  • Let octets be the result of running the steps to get a copy of the bytes held by the buffer source input. Rethrow any exceptions.
  • Return a new key with type binary and value octets.

This means type information is lost, and the key cannot be converted back to its original type when you access cursor.key. I guess Chrome opted to convert the binary type to string, hence '0,127'.

vweevers commented 6 years ago

Nope, hold on, I made a mistake.

vweevers commented 6 years ago

The stringification was a side-effect of the isNaN check because:

> isNaN(Uint8Array.from([0,127]))
true

In actuality, Chrome returns an ArrayBuffer if you access cursor.key. I can work with that :)