React 18 support

[tom-sherman]

Closes #34

This PR includes a small amount of formatting in src/React.res, I can remove that commit if #36 is merged first.

Marked as draft until React 18 is released.

• useSyncExternalStore
• Uncurried version of hooks Likely not needed, let me know if they are
• Tests

[tom-sherman]

We can bikeshed the name, it's a tricky one. The alternative could be to name the parameters so that we can have getServerSnapshot be optional so it doesn't need to passed?

[rickyvetter]

This is interesting to me because there are a bunch of potential future arguments - useSyncExternalStoreWithSelector in the discussion gets wildly long and adding an option to get named optional arguments means it could break in the future if those args get upstreamed into the React export. I think your naming make sense and is probably the safest way to work with these at least until there's more stability for 18 and we see how folks actually use this hook.

[tom-sherman]

I think one option could even be to leave the uSES hooks out for the time being. These hooks I expect to not be very prevalent at all in userland code, but I could be wrong 😅

[bloodyowl]

I'd name the arguments, as there are two unit => 'state callbacks.

[rickyvetter]

Thanks for all of this! I think this looks really good - I want to do some more testing and think a lot of this should probably be duplicated into the Uncurried sub-module for consistency. But will merge soon.

[rickyvetter]

This is interesting to me because there are a bunch of potential future arguments - useSyncExternalStoreWithSelector in the discussion gets wildly long and adding an option to get named optional arguments means it could break in the future if those args get upstreamed into the React export. I think your naming make sense and is probably the safest way to work with these at least until there's more stability for 18 and we see how folks actually use this hook.

[zth]

Shouldn't this be something like:

@module("react")
external useTransition: unit => (bool, @uncurry ((unit => unit) => unit)) = "useTransition"

...?

I've noticed that @uncurry here does not actually uncurry the startTransition function, and that's a problem because startTransition does not work if called via the curry helpers of ReScript in the officially released React 18 (it used to work up until rc-0 I think, but then broke).

@rickyvetter (or anyone else) - do you have any insight/idea about this? Would be great to not have to explicitly uncurry each startTransition call, but maybe that's our only option.

[tom-sherman]

Your suggested type is correct, will update.

The uncurry thing feels like a bug in ReScript tbh 🤔

[tom-sherman]

What's more frustrating about having to add the . in yourself is that the type error is not helpful at all IMO

[tom-sherman]

@TheSpyder informed me on discord that @uncurry in this position has no affect 😕

The only way to support useTransition without manually uncurrying would be to add a helper function:

module Transition: {
  type t
  let isPending: t => bool
  let startTransition: (t, unit => unit) => unit
} = {
  type t = (bool, (. unit => unit) => unit)

  let isPending = transition =>
    switch transition {
    | (pending, _) => pending
    }

  let startTransition = ((_, start), fn) => start(. fn)
}

@module("react")
external useTransition: unit => Transition.t = "useTransition"

// Usage:

let {isPending, startTransition} = module(Transition)

let transition = useTransition()

let _ = transition->isPending

transition->startTransition(() => {
  Js.log("transition")
  ()
})

I'm not a fan of this tbh, it's pretty unprecedented in the API of these bindings and it's zero cost. I'm going to go ahead with the manual uncurried version for now.

[tom-sherman]

@rickyvetter I think the only blocker on merging this is #35 (comment) - would you agree?

Also what do you think about removing the useSES bindings for now?

And shall we do uncurried bindings in a followup? Just so that #46 is unblocked.

[cknitt]

This should be react-dom/client.

[whitchapman]

@cknitt I addressed this in the followup PR #46 which has been sitting in limbo for 2 months. I suggest we merge this PR and then open #46 for review, or maybe @tom-sherman can "steal" the commits in #46 and include them in this PR.

[tom-sherman]

Whatever works, just need a maintainer to get the ball rolling!

@rickyvetter Pls 🙏

[hackwaly]

What's made this PR blocked? I was waiting for react 18 binding so long!

[cristianoc]

See PPX v4:
https://github.com/rescript-lang/rescript-react/pull/49/files#diff-46efb9fafbbb11cf26bd8ac10f0bc5df02ac0e42f593b87cce382e712b8cf950R15

Seems like a good place for this to go.

[cristianoc]

The issue is currently there's nobody specifically maintaining rescript-react per se. So pinging from time to time, as @hackwaly just did, could help things move along.

[cknitt]

@tom-sherman Thanks a lot for your contribution! 👍

I will close this PR as we are finishing the work in #46.