-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Use consistent naming for "returns a Promise" subfeatures #11630
Conversation
This matches the style used for api.HTMLMediaElement.play: https://github.com/mdn/browser-compat-data/blob/b49252cf4c243e1fe1170a359c09937b1c9b75ea/api/HTMLMediaElement.json#L2739-L2741 Only RTCPeerConnection remains using a different structure, but needs a lot of changes so is left out of this update.
@ddbeck can you review? I'm not sure if you want a guideline for this? |
This is interesting. Thankfully it doesn't seem like too many features are affected (if there were lots, I'd give serious consideration to a more fundamental change: there's two distinct APIs—thus two features—that happen to share a name).
RTCPeerConnection uses deprecated features for callbacks, right? I wonder if that's the right way to do it, or to use partial implementation statement on the parent feature (though that's a little anachronistic). Seems like the old definition of the API ought to become less interesting over time, as the Promise version cements as the correct one. That said, it's not a lot of features, so I don't feel strongly about this. If you want to apply these changes and stop there, that's better than what we've got and it's worth doing. So take that up if you like, but I'm not going to require it.
If this were a one-off, I'd say we don't need a guideline, since a) it won't be an ongoing effort to align these features to each other and b) it shouldn't reoccur, on the expectation that new specs won't coming along that will switch from callbacks to promises. That said, this won't be fixed up in a single PR, so I'd like to see either a guideline added to this PR or an issue opened to make a paper trail that ties together the overall effort. Basically, I want a single thing to refer to should a question arise about how to structure something like this. |
I'll have to come back to this after vacation, don't expect anything in the next two weeks. |
There's more of this in #12300, so a guideline is needed now I guess... |
1da5215
to
c1a8446
Compare
c1a8446
to
3a0845f
Compare
@ddbeck this now includes a guideline as well, can you take a look? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you! 🎉
* Bump version to v4.0.5 * Bump known issues * Add release note for #12417 * Add release note for #12454 * Add release note for #12417 * Add release note for #12346 * Add release note for #12455 * Add release note for #12518 * Add release note for #11630 * Add release note for #12300 * Add release note for #12552 * Add release stats * Add release date * Fix a typo
This matches the style used for api.HTMLMediaElement.play:
browser-compat-data/api/HTMLMediaElement.json
Lines 2739 to 2741 in b49252c
Only RTCPeerConnection remains using a different structure, but needs a
lot of changes so is left out of this update.