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

Request for @functionExpression attribute to opt-out from arrow functions #7061

Open
DZakh opened this issue Sep 30, 2024 · 10 comments
Open
Assignees
Milestone

Comments

@DZakh
Copy link
Contributor

DZakh commented Sep 30, 2024

After the PR #6945 function expressions were changed to arrow functions to improve readability and bundle-size. But it also broke this and arguments binding to the function context, which could be accessed via %raw("this") when needed for some advanced logic.

I need it for rescript-schema, so I think introducing an attribute @functionExpression to opt-out from arrow functions would be a good solution.

@DZakh
Copy link
Contributor Author

DZakh commented Sep 30, 2024

@cknitt @cometkim

@cknitt cknitt added this to the v12 milestone Sep 30, 2024
@cknitt cknitt moved this to Ready in ReScript development Sep 30, 2024
@cometkim
Copy link
Member

cometkim commented Sep 30, 2024

Doesn't @meth work?

We also have @this binding. Afaik, it works for both regular function and FFI

@DZakh
Copy link
Contributor Author

DZakh commented Sep 30, 2024

Doesn't @meth work?

We also have @this binding. Afaik, it works for both regular function and FFI

I'll check

@cometkim cometkim self-assigned this Sep 30, 2024
@cometkim
Copy link
Member

I've been exploring this for a bit. rescript-schema seems to be a somewhat special case. I believe most apps have a more explicit and idiomatic solution for binding context without relying on JS this.

ReScript's @this attr support is only compatible with the Js_OO.Callback type, not plain functions because that is the safe way to bind this in "callbacks".

It wouldn't be hard to implement for plain functions either, since there are already primitive implementations, but it would be inherently an unsafe way to write code, so even if we add it, it should be marked as an "unsafe" feature.

@cknitt
Copy link
Member

cknitt commented Oct 4, 2024

I've been exploring this for a bit. rescript-schema seems to be a somewhat special case. I believe most apps have a more explicit and idiomatic solution for binding context without relying on JS this.

Agreed.

It is important that we get rescript-schema to work with v12 though. Is that not feasible at all without this, @DZakh? If so, then I think we should add the requested feature.

@cometkim
Copy link
Member

cometkim commented Oct 4, 2024

It seems like there's a lot of this dependency, and removing it and going back to the type t pattern would require a lot of boilerplate and some breaking changes. rescript-schema looks like they're trying to avoid that and provide a more JS-friendly API.

My suggestion is to expose a primitive API instead of attributes. We already have the necessary implementations; %unsafe_to_method. If we provide a proper namespace for it, we can do something like this:

Js.Unsafe.thisCall((this, arg1, arg2) => {
  ...
})
// -> (('this, 'a, 'b) => 'result) => (('a, 'b) => 'result)

@cknitt
Copy link
Member

cknitt commented Oct 10, 2024

@DZakh Would @cometkim's suggestion work for you?

This is something that you could already test now. %unsafe_to_method is currently exposed as Js_OO.unsafe_to_method.

@DZakh
Copy link
Contributor Author

DZakh commented Oct 16, 2024

Js_OO.unsafe_to_method works for me.

@fhammerschmidt
Copy link
Member

Should we document this somehow?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Ready
Development

No branches or pull requests

4 participants