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

Remove usages of Buffer so we don't need to ship the polyfill #28626

Merged
merged 9 commits into from
Dec 4, 2024

Conversation

t3chguy
Copy link
Member

@t3chguy t3chguy commented Dec 3, 2024

For matrix-org/matrix-js-sdk#4569

Looks like we cannot get rid of Buffer entirely as https://www.npmjs.com/package/bloom-filters requires Buffers until Callidon/bloom-filters#70

@t3chguy t3chguy added the T-Task Tasks for the team like planning label Dec 3, 2024
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>
Signed-off-by: Michael Telatynski <[email protected]>
@t3chguy t3chguy marked this pull request as ready for review December 4, 2024 12:17
@t3chguy t3chguy requested review from a team as code owners December 4, 2024 12:17
@t3chguy t3chguy requested review from florianduros and dbkr December 4, 2024 12:17
@t3chguy t3chguy changed the title Update js-sdk usages around Buffers to avoid needing Buffer polyfill Remove usages of Buffer so we don't need to ship the polyfill Dec 4, 2024
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Nice, although why the clamped arrays?

@t3chguy
Copy link
Member Author

t3chguy commented Dec 4, 2024

Nice, although why the clamped arrays?

That's the signature coming out of rust crypto for QR codes, rather than having to convert on both sides it makes sense to just use the right type to begin with

image

The js-sdk was previously doing stuff like this and it just made no sense, I guess for intercompat with Node's Buffer which can be used interchangeably

Much cleaner to just deal with the right type in the first place

image

@t3chguy t3chguy requested a review from dbkr December 4, 2024 13:23
Copy link
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Oh, fair enough - looks like it's probably the type that the wasm bindings pick by default I guess.

@t3chguy t3chguy merged commit 085854b into develop Dec 4, 2024
48 checks passed
@t3chguy t3chguy deleted the t3chguy/web-friendly-buffers branch December 4, 2024 22:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Task Tasks for the team like planning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants