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

editorial: fix Script.InternalId type #583

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

Lightning00Blade
Copy link
Contributor

@Lightning00Blade Lightning00Blade commented Oct 27, 2023

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


Preview | Diff

@OrKoN OrKoN requested a review from jgraham October 27, 2023 12:44
Copy link
Member

@jgraham jgraham left a comment

Choose a reason for hiding this comment

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

It's defined as a unique integer in the prose, so on its own this change seems wrong (and suggests any change here requires more justification than just "editorial")

@jgraham
Copy link
Member

jgraham commented Oct 27, 2023

"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
Copy link
Contributor

whimboo commented Oct 27, 2023

"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
Copy link
Member

jgraham commented Oct 27, 2023

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
Copy link
Contributor

We are fine with making internalId a UUID.

Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

This would also need an update to the very hard to read sentence:

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

@sadym-chromium
Copy link
Contributor

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.

index.bs Outdated Show resolved Hide resolved
@whimboo whimboo added bug Something isn't working module-script Script module and removed needs-discussion Issues to be discussed by the working group labels Nov 6, 2023
Copy link
Contributor

@whimboo whimboo left a comment

Choose a reason for hiding this comment

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

Looks good to me as well now. Thanks!

@whimboo
Copy link
Contributor

whimboo commented Nov 6, 2023

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

@jgraham jgraham merged commit dbf72c0 into w3c:main Nov 7, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working module-script Script module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants