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

Chrome/Deno/Node/Safari started rejecting ReadableStream*Reader.releaseLock() with pending read requests #25516

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MattiasBuelens
Copy link
Contributor

@MattiasBuelens MattiasBuelens commented Dec 27, 2024

Summary

Update the compatibility data for reject_pending_read_request in both ReadableStreamDefaultReader and ReadableStreamBYOBReader. See whatwg/streams#1168 for context. All major browsers now implement the new behavior.

Test results and supporting details

@github-actions github-actions bot added data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API size:m [PR only] 25-100 LoC changed labels Dec 27, 2024
Copy link
Contributor

@skyclouds2001 skyclouds2001 left a comment

Choose a reason for hiding this comment

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

api/ReadableStreamBYOBReader.json Outdated Show resolved Hide resolved
api/ReadableStreamDefaultReader.json Outdated Show resolved Hide resolved
@github-actions github-actions bot added the data:browsers Data about browsers (versions, release dates, etc). This data is used for validation. label Dec 28, 2024
@github-actions github-actions bot added size:l [PR only] 101-1000 LoC changed and removed size:m [PR only] 25-100 LoC changed labels Dec 28, 2024
@MattiasBuelens MattiasBuelens changed the title Update reject_pending_read_request Update reject_pending_read_request and merge with releaseLock() Dec 28, 2024
@MattiasBuelens
Copy link
Contributor Author

MattiasBuelens commented Dec 28, 2024

I went ahead and removed the reject_pending_read_request entry, merging it into the existing releaseLock() entry. I think this better reflects that this behavior change only affects the releaseLock() method, and that older versions with the old behavior are only "partially" implementing the standard.

I believe this removal is warranted under the rules for removing irrelevant features, but I'm open to discuss. Maybe @hamishwillee could chime in, as they added the feature in the first place in #16919? 🙂

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

The changes up to d78a916 LGTM, but afaik we don't "merge" spec change subfeatures, partly because it makes the support history harder to read across browsers, and because there can be multiple spec changes that don't get implemented at the same time, which would make the notes even harder to understand.

@MattiasBuelens MattiasBuelens changed the title Update reject_pending_read_request and merge with releaseLock() Update reject_pending_read_request Jan 2, 2025
@github-actions github-actions bot added size:m [PR only] 25-100 LoC changed and removed size:l [PR only] 101-1000 LoC changed labels Jan 2, 2025
@MattiasBuelens
Copy link
Contributor Author

@caugner Fair enough, I've reverted the PR back to that commit.

Copy link
Contributor

@caugner caugner left a comment

Choose a reason for hiding this comment

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

Thank you, the data LGTM. Since the features now have good descriptions, we can now make the notes more concise.

"version_added": false,
"notes": "`releaseLock()` throws if there are pending read requests (rather than pending read requests being rejected)."
"version_added": "105",
"notes": "Before version 105, `releaseLock()` would throw if there are pending read requests (rather than pending read requests being rejected)."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"notes": "Before version 105, `releaseLock()` would throw if there are pending read requests (rather than pending read requests being rejected)."
"notes": "Before version 105, `releaseLock()` throws instead of rejecting."

"version_added": false,
"notes": "`releaseLock()` throws if there are pending read requests (rather than pending read requests being rejected)."
"version_added": "105",
"notes": "Before version 105, `releaseLock()` would throw if there are pending read requests (rather than pending read requests being rejected)."
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"notes": "Before version 105, `releaseLock()` would throw if there are pending read requests (rather than pending read requests being rejected)."
"notes": "Before version 105, `releaseLock()` throws instead of rejecting."

@caugner caugner changed the title Update reject_pending_read_request Chrome/Deno/Node/Safari started rejecting ReadableStream*Reader.releaseLock() with pending read requests Jan 6, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API data:browsers Data about browsers (versions, release dates, etc). This data is used for validation. size:m [PR only] 25-100 LoC changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants