Skip to content

Add base types of ArrayBuffer, SharedArrayBuffer #6683

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

Closed
wants to merge 3 commits into from

Conversation

cometkim
Copy link
Member

@cometkim cometkim commented Mar 17, 2024

TypedArray.prototype.buffer is polymorphic. ArrayBuffer or SharedArrayBuffer

Atomics is mostly used with Int32Array on SAB.

It's quite a corner case, but not a common use case, so I think it's not practical to make the module structure or type variants represent it strictly.

@cometkim cometkim force-pushed the sab-atomics branch 2 times, most recently from 6b88b9f to 6e73d3a Compare March 17, 2024 23:41
@cometkim cometkim marked this pull request as ready for review March 18, 2024 12:22
@cometkim
Copy link
Member Author

cometkim commented Mar 18, 2024

This should be moved into Core. Onlt type t should be remained here

@cometkim cometkim force-pushed the sab-atomics branch 6 times, most recently from ce2c6b9 to 7f64c21 Compare March 18, 2024 15:00
@cometkim cometkim changed the title Add bindings to updated ArrayBuffer, SharedArrayBuffer, and Atomics Add base types of ArrayBuffer, SharedArrayBuffer Mar 18, 2024
@@ -0,0 +1,6 @@
type t = {
Copy link
Member

Choose a reason for hiding this comment

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

I'd suggest marking the record as private so it's not possible to create by hand.

Copy link
Member

Choose a reason for hiding this comment

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

Since not every record with the fields is the ArrayBuffer

Copy link
Member Author

Choose a reason for hiding this comment

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

That's basically prevented by nominal typing

Copy link
Member

Choose a reason for hiding this comment

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

I mean to prevent the code like this:

let myArrayBuffer: Js.ArrayBuffer.t = {
  byteLenght: 2,
  maxByteLength: 4,
  detached: false,
  resizable: false
}

Copy link
Member Author

Choose a reason for hiding this comment

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

If we don't want that, we shouldn't use the binding pattern of exposing getters as record fields for every this-bounded object.

I thought it was already a widely used (even encouraged) pattern, more for practicality than strictness. for example: TheSpyder/rescript-webapi#78 (comment)

Copy link
Member

Choose a reason for hiding this comment

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

I personally use private for my projects. It kind of solves the problem:

type t = private {
  byteLength: int,
  maxByteLength: int,
  detached: bool,
  resizable: bool,
}

But I see one downside compared to getter pattern:

You need to annotate the value when you access the field

let fn = (arrayBuffer: ArrayBuffer.t) => arrayBuffer.byteLenght
// vs
let fn = (arrayBuffer) => arrayBuffer->Js.ArrayBuffer.byteLength

It's fine for this case but when you have a type with a lot of complicated type arguments I prefer not to write annotations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can just leave type t here, but what about the Core's direction? @zth

I'll tune in to that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Wait, so does it make sense to provide compatibility with Js' type in the first place?

@cometkim
Copy link
Member Author

ok I'm going to close this and move it to Core entirely. If the direction is to remove JS in v12, I think there is no reason to change here, regardless of whether type t is extended or not. The only reason is compatibility with v11 but that can wait.

@cometkim cometkim closed this Mar 25, 2024
@cometkim cometkim deleted the sab-atomics branch April 2, 2024 15:20
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