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

Update typing of typed arrays to support Wasm, and test Typed_array.Bytes #1656

Merged
merged 12 commits into from
Sep 13, 2024

Conversation

OlivierNicole
Copy link
Contributor

@OlivierNicole OlivierNicole commented Jul 30, 2024

Note: This PR depends on #1649 and is based on top of it. The feature change consists in the last two commits. edit: #1649 has been merged.

This:

  • Updates the typing of Typed_array so that they work with the Wasm backend as well
  • Adds tests for the new Typed_array.Bytes module.

This is part of a series of PRs intending to reduce the diff between js_of_ocaml and wasm_of_ocaml (see ocaml-wasm/wasm_of_ocaml#47).

@hhugo
Copy link
Member

hhugo commented Jul 30, 2024

Turning the PR into a draft until the underlying PR is merged.

@hhugo hhugo marked this pull request as draft July 30, 2024 12:19
@OlivierNicole OlivierNicole marked this pull request as ready for review September 3, 2024 14:27
@OlivierNicole
Copy link
Contributor Author

I rebased and marked the PR as ready for review.

@hhugo
Copy link
Member

hhugo commented Sep 4, 2024

Any idea how much breakage the change of API generates ?

@hhugo
Copy link
Member

hhugo commented Sep 4, 2024

It seems we could now complete the api with BigInt64Array and BigUint64Array. One could do that in a subsequent PR.
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/BigInt64Array

@hhugo
Copy link
Member

hhugo commented Sep 4, 2024

The PR definitely need a changelog entry

@OlivierNicole
Copy link
Contributor Author

Any idea how much breakage the change of API generates ?

I’m not sure. I suppose we could run an opam-health-check on the PR.

@hhugo
Copy link
Member

hhugo commented Sep 13, 2024

@OlivierNicole, can you look at Jerome suggestion ? and rebase and squash commits ?

@hhugo hhugo merged commit 818dcd6 into ocsigen:master Sep 13, 2024
17 checks passed
@OlivierNicole
Copy link
Contributor Author

Oh, I was rebasing, but you merged before I could update the PR.

@OlivierNicole
Copy link
Contributor Author

In any case, thank you for the review and @vouillon for the explanations!

@hhugo
Copy link
Member

hhugo commented Sep 13, 2024

I ended up squashing during the merge

@hhugo
Copy link
Member

hhugo commented Nov 23, 2024

This breaks ezjs_crypto. I am adding a constraint in opam. See ocaml/opam-repository@e1c4e8e

@hhugo hhugo added the wasm label Nov 25, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants