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

useState uncurry #860

Draft
wants to merge 5 commits into
base: 19
Choose a base branch
from
Draft

useState uncurry #860

wants to merge 5 commits into from

Conversation

davesnx
Copy link
Member

@davesnx davesnx commented Nov 19, 2024

Depends on #859 replace-testing-libraries, that depends on #846 React 19 new APIs


Since React 19, useState has changed (facebook/react#31578), (facebook/react#31587) (facebook/react#31598)

Example with current useState:

    let onClick = _ => {
      setState(_ => 22);
    };
  const onClick = function (param) {
    Curry._1(setState, (function (param) {
            return 22;
          }));
  };
function _1(o, a0) {
  const arity = o.length;
  if (arity === 1) {
    return o(a0);
  } else {
    switch (arity) {
      case 1 :
          return o(a0);
      case 2 :
          return function (param) {
            return o(a0, param);
          };
      case 3 :
          return function (param, param$1) {
            return o(a0, param, param$1);
          };
      // ...
      default:
        return app(o, [a0]);
    }
  }
}

setState's arity is 1 -> setState(function (param) { return 22; })

https://melange.re/unstable/playground/?language=Reason&code=bW9kdWxlIEdyZWV0aW5nID0gewogIFtAcmVhY3QuY29tcG9uZW50XQogIGxldCBtYWtlID0gKCkgPT4gewogICAgbGV0IChzdGF0ZSwgc2V0U3RhdGUpID0gUmVhY3QudXNlU3RhdGUoKCkgPT4gMzMpOwogICAgbGV0IG9uQ2xpY2sgPSBfID0%2BIHsKICAgICAgc2V0U3RhdGUoXyA9PiAyMik7CiAgICB9OwogICAgPGJ1dHRvbiBvbkNsaWNrPgogICAgICB7UmVhY3Quc3RyaW5nKCJIZWxsbyEiICsrIEludC50b19zdHJpbmcoc3RhdGUpKX0KICAgIDwvYnV0dG9uPjsKICB9Owp9OwoKbGV0IGVsZW1lbnQgPSBSZWFjdERPTS5xdWVyeVNlbGVjdG9yKCIjcHJldmlldyIpOwoKbGV0ICgpID0KICBzd2l0Y2ggKGVsZW1lbnQpIHsKICB8IFNvbWUoZWxlbWVudCkgPT4KICAgIGxldCByb290ID0gUmVhY3RET00uQ2xpZW50LmNyZWF0ZVJvb3QoZWxlbWVudCk7CiAgICBSZWFjdERPTS5DbGllbnQucmVuZGVyKHJvb3QsIDxHcmVldGluZyAvPik7CiAgfCBOb25lID0%2BCiAgICBKcy5Db25zb2xlLmVycm9yKAogICAgICAiRmFpbGVkIHRvIHN0YXJ0IFJlYWN0OiBjb3VsZG4ndCBmaW5kIHRoZSAjcHJldmlldyBlbGVtZW50IiwKICAgICkKICB9Owo%3D&live=off

In React 19 (TBD link) setState arity is 2 balblaba

The easier to fix is to ensure we apply uncurry manually to the binding and have a breaking change:

https://melange.re/unstable/playground/?language=Reason&code=W0BtZWwubW9kdWxlICJyZWFjdCJdCmV4dGVybmFsIHVzZVN0YXRlOgogIChbQG1lbC51bmN1cnJ5XSAodW5pdCA9PiAnc3RhdGUpKSA9PiAoJ3N0YXRlLCAoLiAoJ3N0YXRlID0%2BICdzdGF0ZSkpID0%2BIHVuaXQpID0KICAidXNlU3RhdGUiOwoKbW9kdWxlIEdyZWV0aW5nID0gewogIFtAcmVhY3QuY29tcG9uZW50XQogIGxldCBtYWtlID0gKCkgPT4gewogICAgbGV0IChzdGF0ZSwgc2V0U3RhdGUpID0gdXNlU3RhdGUoKCkgPT4gMzMpOwogICAgbGV0IG9uQ2xpY2sgPSBfID0%2BIHsKICAgICAgc2V0U3RhdGUoLiBfID0%2BIDIyKTsKICAgIH07CiAgICA8YnV0dG9uIG9uQ2xpY2s%2BCiAgICAgIHtSZWFjdC5zdHJpbmcoIkhlbGxvISIgKysgSW50LnRvX3N0cmluZyhzdGF0ZSkpfQogICAgPC9idXR0b24%2BOwogIH07Cn07CgpsZXQgZWxlbWVudCA9IFJlYWN0RE9NLnF1ZXJ5U2VsZWN0b3IoIiNwcmV2aWV3Iik7CgpsZXQgKCkgPQogIHN3aXRjaCAoZWxlbWVudCkgewogIHwgU29tZShlbGVtZW50KSA9PgogICAgbGV0IHJvb3QgPSBSZWFjdERPTS5DbGllbnQuY3JlYXRlUm9vdChlbGVtZW50KTsKICAgIFJlYWN0RE9NLkNsaWVudC5yZW5kZXIocm9vdCwgPEdyZWV0aW5nIC8%2BKTsKICB8IE5vbmUgPT4KICAgIEpzLkNvbnNvbGUuZXJyb3IoCiAgICAgICJGYWlsZWQgdG8gc3RhcnQgUmVhY3Q6IGNvdWxkbid0IGZpbmQgdGhlICNwcmV2aWV3IGVsZW1lbnQiLAogICAgKQogIH07Cg%3D%3D&live=off

To not have a breaking change, we probably need to bring some runtime (similar to #698):

[@mel.module "react"]
external useStateUncurry:
  ([@mel.uncurry] (unit => 'state)) =>
  ('state, (. ('state => 'state)) => unit) =
  "useState";

let useState = initialState => {
  let (state, setState) = useStateUncurry(initialState);
  let setState = fn => setState(. fn);
  (state, setState);
};

This would re-create a new setState function each component render, so it's a good idea to wrap it with React.useCallback1 to ensure Object.is(setState, setState) bails out from this behaviour

let useState = initialState => {
  let (state, setState) = useStateUncurry(initialState);
  let setState = React.useCallback1(fn => setState(. fn), [|setState|]);
  (state, setState);
};

@anmonteiro
Copy link
Member

Does this actually fix anything? I was trying to understand what's the actual issue here and I don't think changing it to uncurried makes the problem go away:

external useState :
  ((unit -> 'state)[@mel.uncurry]) ->
  'state * ((('state -> 'state)[@u]) -> unit) = "useState"
[@@mel.module "react"]

let x, setX = useState (fun _ -> 22)
let () = setX (fun [@u] _ -> 23)
$ melc -ppx melppx x.ml
// Generated by Melange
'use strict';

const Curry = require("melange.js/curry.js");
const React = require("react");

const match = React.useState(function () {
      return 22;
    });

const setX = match[1];

Curry._1(setX, (function (param) {
        return 23;
      }));

const x = match[0];

exports.x = x;
exports.setX = setX;
/* match Not a pure module */

@anmonteiro
Copy link
Member

anmonteiro commented Nov 23, 2024

BTW, my current understanding of why Melange inserts the Curry._1(...) call is because 'state -> 'state is a pretty interesting type for setState ('state could itself be a function!) -- it'd be nice to be able to tell melange to stop after the first application (instead of using Curry._1).

@anmonteiro
Copy link
Member

OK, I understand the issue now. it's rather like React.Uncurried.

Base automatically changed from replace-testing-libraries to 19 November 25, 2024 17:36
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