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

transaction should either take DOMStringList or DOMStringList needs to be iterable #85

Closed
rniwa opened this issue Aug 20, 2016 · 13 comments
Labels
Milestone

Comments

@rniwa
Copy link

rniwa commented Aug 20, 2016

transaction method on IDBDatabase takes a DOMString or a sequence<DOMString> but DOMStringList is not iterable.

The consequence of this is that, you can’t call transaction() with the return value of objectStoreNames method. This is not Web incompatible as far as I know.

@inexorabletash
Copy link
Member

In the past I think this was covered by WebIDL accepting anything with length and indexed properties for sequence arguments, which DOMStringList satisfied. I believe that changed?

Anyway, @rniwa is correct about the use case and I think Blink even has tests for this. (I'm on my phone at the moment.) so we should fix in spec-land somehow, either making DOMStringList iterable or changing the method.

@inexorabletash inexorabletash added this to the V2 milestone Aug 20, 2016
@inexorabletash
Copy link
Member

@aliams, @wanderview - do you have a preference between:

  • Make DOMStringList have iterable<DOMString>; or
  • Make transaction()'s signature be transaction((DOMString or sequence<DOMString> or DOMStringList) and add logic for DOMStringList in the method algorithm

The former is more consistent with other platform types and with our eventual hope to replace it with FrozenArrayWithContains<DOMString> (or whatever), but does add more to a deprecated type.

The latter is a more scoped change.

I'm pretty sure Blink internally still checks for length and indexed properties, as previously specified in WebIDL, but either approach is likely to be web-compatible.

@inexorabletash
Copy link
Member

@beidson too - same question as previous comment.

@aliams
Copy link

aliams commented Aug 22, 2016

Considering the goal to eventually remove DOMStringList, I'd say we should go with option 1 so that we're not adding extra logic to handle DOMStringList that will just need to be removed later. I'm not as worried about making DOMStringList iterable because it will get us one step closer to how developers will use a FrozenArray.

@inexorabletash
Copy link
Member

FYI @domenic @annevk

@annevk
Copy link
Member

annevk commented Dec 16, 2016

Yeah, making DOMStringList iterable seems like the way to go, even if we have to keep it.

@annevk
Copy link
Member

annevk commented Dec 17, 2016

My preference would be to fix this as a follow-up commit to whatwg/html#2192 with corresponding WPT changes.

@inexorabletash
Copy link
Member

sgtm.

I have a blink CL I haven't landed because I didn't want to have the "it's a deprecated API we're trying to remove, why are we adding to it?" fight.

@inexorabletash
Copy link
Member

Spec-wise, we should decide on what to "domintro" for iterable<>. I notice that DOMTokenList's docs don't mention the methods that get bolted on.

@annevk
Copy link
Member

annevk commented Dec 20, 2016

Hmm good point. domintro should probably mention all. I wonder if we can get some domintro automation into bikeshed to make things a little less tedious.

@inexorabletash
Copy link
Member

Turns out this is moot. Per WebIDL

If the interface has any of the following:

  • ...
  • an indexed property getter and an integer-typed attribute named “length”
  • ...

then a property must exist whose name is the @@iterator symbol...

TIL Blink and Gecko already implement this. Wheee!

Blink tosses DOMStringList into the union but that's an implementation detail - the spec is fine.

@annevk
Copy link
Member

annevk commented Jan 14, 2017

I think adding iterable<> might still be worth it, especially if user agents have codegen for it, but not really a priority.

@inexorabletash
Copy link
Member

Agreed. I have a local patch for Blink; if it appeared in the spec I'd go through our "intent" process to ship it, but not a priority to push for it.

webkit-commit-queue pushed a commit to WebKit/WebKit that referenced this issue Oct 20, 2021
https://bugs.webkit.org/show_bug.cgi?id=231997

Reviewed by Chris Dumez.

DOMStringList is also iterable in WebKit now, so we don't need to keep this definition for compatibility (see
w3c/IndexedDB#85 (comment)).

Covered by existing test storage/indexeddb/transaction-basics.html, which verifies that DOMStringList can be
argument for IDBDatabase.transaction().

* Modules/indexeddb/IDBDatabase.idl:


Canonical link: https://commits.webkit.org/243292@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@284550 268f45cc-cd09-0410-ab3c-d52691b4dbfc
bertogg pushed a commit to Igalia/webkit that referenced this issue Oct 21, 2021
https://bugs.webkit.org/show_bug.cgi?id=231997

Reviewed by Chris Dumez.

DOMStringList is also iterable in WebKit now, so we don't need to keep this definition for compatibility (see
w3c/IndexedDB#85 (comment)).

Covered by existing test storage/indexeddb/transaction-basics.html, which verifies that DOMStringList can be
argument for IDBDatabase.transaction().

* Modules/indexeddb/IDBDatabase.idl:


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@284550 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants