Skip to content

Support generic Uint8Array<ArrayBuffer> #1944

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

Merged
merged 12 commits into from
Apr 18, 2025
Merged

Support generic Uint8Array<ArrayBuffer> #1944

merged 12 commits into from
Apr 18, 2025

Conversation

rbuckton
Copy link
Member

@rbuckton rbuckton commented Mar 21, 2025

This updates the DOM to use Uint8Array<ArrayBuffer> in some cases where the specification expects it. This addresses an error reported in microsoft/TypeScript#61432 for ajayyy/SponsorBlock.

This now also fixes AllowSharedBufferSource, which fails to include SharedArrayBuffer.

Fixes microsoft/TypeScript#61480

Verified

This commit was signed with the committer’s verified signature.
rbuckton Ron Buckton
@saschanaz
Copy link
Contributor

This should use ArrayBuffer by default than just overriding some cases, and use SAB+AB when [AllowShared] is used.

Verified

This commit was signed with the committer’s verified signature.
rbuckton Ron Buckton

Verified

This commit was signed with the committer’s verified signature.
rbuckton Ron Buckton
@rbuckton
Copy link
Member Author

@sandersn: The implementation changed significantly. Can you take another look?

@saschanaz
Copy link
Contributor

saschanaz commented Mar 28, 2025

This adds 5.6, but is it worth? Can it be folded into 5.7? Err, never mind, there's no 5.7 now.

@rbuckton
Copy link
Member Author

rbuckton commented Mar 28, 2025

This adds 5.6, but is it worth? Can it be folded into 5.7? Err, never mind, there's no 5.7 now.

There are still package.json entries for <=5.5 and <=5.6. The <=5.5 entry handles usage in tsc prior to 5.6, which introduced IteratorObject. The <=5.6 entry handles usage in tsc prior to 5.7, which introduced generic typed arrays. The default types used will be for 5.7 and later.

Unfortunately, 5.6 cannot be folded into 5.7+ as use of Uint8Array<T> would fail in 5.6.

sandersn
sandersn previously approved these changes Mar 31, 2025
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Makes sense to me. One question about the change to WritableStream in overridingtypes.json to make sure I understood it correctly.

},
"writable": {
"name": "writable",
"readonly": true,
"overrideType": "WritableStream<string>"
"type": "WritableStream",
Copy link
Member

Choose a reason for hiding this comment

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

The output doesn't change that I could tell, so I guess this is an update to use the new subtype property for consistency?

Copy link
Contributor

Choose a reason for hiding this comment

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

Refraining from overrideType is a good change 👍🏻 (use of overrideType disables type dependency tracking)

Copy link
Contributor

Choose a reason for hiding this comment

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

(Though I would have preferred if this kind of change is file as a separate PR, as mixing it makes harder to understand it)

},
"writable": {
"name": "writable",
"readonly": true,
"overrideType": "WritableStream<string>"
"type": "WritableStream",
Copy link
Contributor

Choose a reason for hiding this comment

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

(Though I would have preferred if this kind of change is file as a separate PR, as mixing it makes harder to understand it)

Verified

This commit was signed with the committer’s verified signature.
rbuckton Ron Buckton
Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

Still would like to change gitattributes to use only LF, looks good otherwise!

sandersn
sandersn previously approved these changes Apr 1, 2025

Verified

This commit was signed with the committer’s verified signature.
rbuckton Ron Buckton
@jakebailey
Copy link
Member

What happened to this PR? Main TS uses this, right?

@saschanaz
Copy link
Contributor

I believe it's just waiting for you to merge this 😛

@jakebailey
Copy link
Member

There are merge conflicts but ones I assume will be fixed by regenerating?

@saschanaz
Copy link
Contributor

Please do so 🙂 (well if I had the permission... 😜)

@jakebailey
Copy link
Member

lol, of course.

image

@sandersn @rbuckton is this PR good to go?

@jakebailey jakebailey requested a review from sandersn April 17, 2025 17:55
@jakebailey jakebailey requested a review from sandersn April 18, 2025 18:20
@jakebailey jakebailey merged commit 652339e into main Apr 18, 2025
8 checks passed
@jakebailey jakebailey deleted the generic-typedarrays branch April 18, 2025 23:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AllowSharedBufferSource definition is incorrect
5 participants