search

w3c / webdriver-bidi

Bidirectional WebDriver protocol for browser automation
https://w3c.github.io/webdriver-bidi/
336 stars 35 forks source link

editorial: fix Script.InternalId type #583

Closed Lightning00Blade closed 8 months ago

Lightning00Blade commented 8 months ago

We always define Ids as text, this is an exception, we should change this. Note: Both Chromium and Firefox return strings atm

Preview | Diff

jgraham commented 8 months ago

"Firefox and Chrome return strings" seems like a good justification BTW. But do they return strings that represent integers? If so it seems unfortunate because people will probably do parseInt on those strings and we might need to specify that it's the string representation of an integer.

whimboo commented 8 months ago

"Firefox and Chrome return strings" seems like a good justification BTW. But do they return strings that represent integers? If so it seems unfortunate because people will probably do parseInt on those strings and we might need to specify that it's the string representation of an integer.

Oh, we do it totally wrong then. In Firefox we send a UUID as value for internalId so we probably wouldn't mind such a change. Interestingly no-one reported that problem yet, so I assume it's most likely not really in use as a number by clients.

jgraham commented 8 months ago

If we're returning a UUID already that somewhat alleviates the concern (we can just change the prose to say "globally unique string"). My worry is that in cases where we just say "return a string" and everyone is returning (say) an integer then clients depend on it being an integer.

I still worry a bit that we'll find cases where any client returning a string in a known, but unspecified, format will cause authors to depend on that format (e.g. Chrome returning the string representation of an integer will cause people to write tests that depend on the string representing an integer), but so far I don't think we have consensus that all ids should be UUIDs to avoid this problem.

sadym-chromium commented 8 months ago

We are fine with making internalId a UUID.

sadym-chromium commented 8 months ago

Let internal id be a unique across the internalId fields of the values of serialization internal map integer.

Let internal id be the string representation of a UUID based on truly random, or pseudo-random numbers.

whimboo commented 8 months ago

@jgraham can you please update your review status? Merging is blocked because you requested changes. Thanks!