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

IndexedDB 2.0 features review #153

Closed
3 tasks done
pwnall opened this issue Feb 17, 2017 · 6 comments
Closed
3 tasks done

IndexedDB 2.0 features review #153

pwnall opened this issue Feb 17, 2017 · 6 comments
Assignees

Comments

@pwnall
Copy link

pwnall commented Feb 17, 2017

Hello TAG!

I'm requesting a TAG review of:

Further details (optional):

You should also know that...

We have submitted tests covering all the features above to web-platform-tests. Chrome (with the Experimental Web Platform Features flag) passes all the test cases (159/159). Firefox (nightly) passes over 98.7% (157/159) of the test cases, and Safari (Tech Preview 23) passes over 99.3% (158/159) of the test cases.

Firefox has already shipped all the IndexedDB 2.0 features mentioned above, based on the Editor's Draft specification, in Firefox 51. Safari is about to ship the features above
in Safari 10.1, which is included in the OS X 10.12.4 beta. We (Chrome) have implemented the features under the Experimental Web Platform Features flag, and plan to ship them following this TAG review.

We'd prefer the TAG provide feedback as (please select one):


Please preview the issue and check that the links work before submitting

For background, some decent explainers:

https://github.com/w3c/ServiceWorker/blob/master/explainer.md
https://github.com/zkoch/paymentrequest/blob/gh-pages/docs/explainer.md
https://github.com/WICG/IntersectionObserver/blob/gh-pages/explainer.md (although this one includes IDL, which an explainer should not)

@cynthia cynthia self-assigned this Feb 21, 2017
@cynthia
Copy link
Member

cynthia commented Feb 21, 2017

Blunt first pass comments from someone without much background on the design...

Just out of plain curiosity: since binary keys expect the underlying type information to be discarded when stored, would it be a problem if two different array buffers containing the same data at binary level work as the same key?

I'm not sure what the real world implications of this side effect would be (I suspect minimal) but it could in theory be a problem in extreme corner cases.

I've looked at IndexedDB/issues/#22 and the use case behind allowing renaming wasn't noted - what is the use case for this?

I need a bit more time to look at key generators, will post an update soon.

@inexorabletash
Copy link

...since binary keys expect the underlying type information to be discarded when stored...

There may be developer confusion that e.g. a Uint8Array and Float32Array consisting of the same backing bytes have the same value as a key. (This was touched on in the linked discussion). This pattern shows up in a few other places in the platform where APIs consume binary data; we eventually converged on the conventions specified in WebIDL's "get a copy of the bytes..." and the BufferSource abstract type. So the platform is consistent, but again there is the possibility of developer confusion.

...use case behind allowing renaming...

Most of the discussion around store/index renaming took place offline, and I failed to capture the intents there. (This also included guidance from @annevk on using setter rather than e.g. setName or anything awkard like that.)

This blog post by @bevis-tseng covers the use cases pretty well: https://hacks.mozilla.org/2016/10/whats-new-in-indexeddb-2-0/

...key generators...

Nothing (intentionally) new with key generators in 2.0 vs. 1.0, although we have identified a spec gap and interop issues with edge cases, discussed at w3c/IndexedDB#147 - more eyes on everything welcome!

@pwnall
Copy link
Author

pwnall commented Feb 21, 2017

Here is a rationale for renaming.

Applications that use the active record pattern have a 1:1 matching between stores and class names, for example Widget instances are persisted in a "widgets" store. When performing a code refactoring that renames the class, it is desirable to be able to rename the store as well, in order to maintain the pattern. Naming is hard, so the engineers of a 2-3-year old app are likely to be able to find a better name for at least one class with persistent instances.

I authored homework submission software that has been used for for 9+ years, and I've renamed a couple of models.

@cynthia
Copy link
Member

cynthia commented Feb 22, 2017

Closed as LGTM as per discussed on 2/22 call.

@cynthia cynthia closed this as completed Feb 22, 2017
@slightlyoff
Copy link
Member

Per today's discussion, closing this with an "LGTM".

Thanks again to @pwnall for the thoughtful and detailed review request. In future, we'd love to see these features much sooner in the design phase. We don't bite!

@pwnall
Copy link
Author

pwnall commented Feb 22, 2017

Thank you very much for the quick turnaround time!

@slightlyoff Point noted! I'll definitely get in touch sooner when designing features.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants