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

Switch from ArrayBuffer to ByteArray #187

Merged
merged 2 commits into from
Dec 17, 2024
Merged

Conversation

jhugman
Copy link
Owner

@jhugman jhugman commented Dec 16, 2024

According to The Big O of Code Reviews, this is a O(n) change.

ubrn lowers most types to an ArrayBuffer and then maps it to a jsi::ArrayBuffer, and then on to RustBuffer.
wasm-bindgen maps a Uint8Array on to a Vec<u8>.

In order to lower types to a Uint8Array for WASM and ArrayBuffer for JSI, some amount of bundler violence was needed. Given we're using multiple bundlers to package up the frontend Typescript, and we're not really in control of what bundler the developer uses, this doesn't seem like a viable approach.

Instead, this PR changes ubrn to lower types down to a Uint8Array, so that both WASM and React Native use the same underlying representation of byte arrays.

Unfortunately, jsi doesn't expose API for typed arrays (it may in the future), so we look up the buffer property to get an underlying ArrayBuffer.

On the way back, an object of type { buffer: ArrayBuffer; } is returned, which is enough to make the uniffi work and the typescript compiler think it that a typed array has come back from Rust/C++.

Most of this PR was mechanical refactoring (e.g. ArrayBuffer renamed to UniffiByteArray) but there are enough manual changes which merit it being O(n).

@jhugman jhugman requested a review from zzorba December 16, 2024 20:08
Copy link
Collaborator

@zzorba zzorba left a comment

Choose a reason for hiding this comment

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

LGTM

This is probably something we should document in the changelog on the next release, given there's a type change? Should we make a spot to accumulate those?

@jhugman jhugman force-pushed the jhugman/jsi/switch-to-uint8array branch from d9347d1 to 8631648 Compare December 17, 2024 14:17
@jhugman jhugman merged commit 0ea522e into main Dec 17, 2024
18 checks passed
@jhugman jhugman deleted the jhugman/jsi/switch-to-uint8array branch December 17, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants