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

Add back Js_typed_array #6891

Merged
merged 3 commits into from
Jul 19, 2024
Merged

Conversation

cknitt
Copy link
Member

@cknitt cknitt commented Jul 18, 2024

No description provided.

@cknitt cknitt changed the title Add back js typed array Add back Js_typed_array Jul 18, 2024
@cknitt cknitt mentioned this pull request Jul 18, 2024
@zth
Copy link
Collaborator

zth commented Jul 18, 2024

What about adding back just the types and not the implementations, what do you think about that?

@cknitt
Copy link
Member Author

cknitt commented Jul 18, 2024

I'd prefer to to keep backwards compatibility as much as possible. So keeping everything except the externals using @bs.send.pipe seems fine to me. (Ideally we would reimplement all those functions based on @send like @cristianoc did for other modules, but there is really a lot of them here, so that would be a lot of work. But could still be done if we find a volunteer. 🙂)

@zth
Copy link
Collaborator

zth commented Jul 18, 2024

Cool!

@cristianoc
Copy link
Collaborator

I'd prefer to to keep backwards compatibility as much as possible. So keeping everything except the externals using @bs.send.pipe seems fine to me. (Ideally we would reimplement all those functions based on @send like @cristianoc did for other modules, but there is really a lot of them here, so that would be a lot of work. But could still be done if we find a volunteer. 🙂)

I kept 100% compatibility, but that requires all that tedious repetitive work from e.g. Js_array.res:

@send
external copyWithinFrom: (t<'a>, ~to_: int, ~from: int) => 'this = "copyWithin"
let copyWithinFrom = (~to_, ~from, obj) => copyWithinFrom(obj, ~to_, ~from)

Copilot and autocomplete help, but it's still tedious.
Not sure it's worth for this.

@cknitt
Copy link
Member Author

cknitt commented Jul 19, 2024

@cristianoc @zth Ok to proceed with this PR as is?

@cknitt cknitt requested review from cristianoc and zth July 19, 2024 06:24
@zth
Copy link
Collaborator

zth commented Jul 19, 2024

Go for it!

Copy link
Collaborator

@cristianoc cristianoc left a comment

Choose a reason for hiding this comment

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

Perhaps a quick line in the changelog describing the situation.

@cknitt cknitt requested review from cristianoc and zth July 19, 2024 08:45
@cknitt cknitt merged commit 6546a77 into rescript-lang:master Jul 19, 2024
19 checks passed
@cknitt cknitt deleted the add-back-js-typed-array branch July 19, 2024 14:22
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.

3 participants