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

Support for React 19 #846

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from
Draft

Support for React 19 #846

wants to merge 37 commits into from

Conversation

davesnx
Copy link
Member

@davesnx davesnx commented Jul 16, 2024

This PR adds some of the new functions on React 19.

src/FormData.ml Outdated Show resolved Hide resolved
src/ReactDOM.re Outdated Show resolved Hide resolved
src/FormData.ml Outdated Show resolved Hide resolved
src/React.re Outdated Show resolved Hide resolved
* 'main' of github.com:/reasonml/reason-react:
  fix: type of pipeable stream to allow objects with keys (#854)
  reason-react-ppx: + lower bound in ocaml
  add missing entries to changelog
  Fix multi-child fragment (#852)
  update compiler version in makefile cmd (#851)
  Add locations-check test (#844)
  fix: re-enable failing tests + fix location tests (#850)
  test: repro #840 (#842)
  Add CSS Box Alignment Module Level 3 (#847)
@davesnx davesnx mentioned this pull request Nov 19, 2024
src/React.re Outdated Show resolved Hide resolved
src/React.re Show resolved Hide resolved
Comment on lines +587 to +590
[@mel.module "react"]
external useTransitionAsync:
unit => (bool, callbackAsync(callbackAsync(unit, unit), unit)) =
"useTransition";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prob out of scope, but now that new bindings are being added, and given they're new and experimental, it's good chance to use chatgpt to add comments to them? i got this for this one with some tweaking:

Suggested change
[@mel.module "react"]
external useTransitionAsync:
unit => (bool, callbackAsync(callbackAsync(unit, unit), unit)) =
"useTransition";
/**
* `useTransitionAsync` is a binding to React's `useTransition` hook.
*
* This hook lets you update state without blocking the UI.
*
* ### Return Value
* - `(bool, callbackAsync(callbackAsync(unit, unit), unit))`: A tuple containing:
* - `bool`: Whether the transition is pending.
* - A function to start the transition.
*
* ### Example
* ```reason
* let (isPending, startTransition) = useTransitionAsync();
*
* /* Start a transition */
* startTransition(() => {
* /* Deferred update logic */
* });
*
* if (isPending) {
* /* Show loading indicator */
* }
* ```
*/
[@mel.module "react"]
external useTransitionAsync:
unit => (bool, callbackAsync(callbackAsync(unit, unit), unit)) =
"useTransition";

Copy link
Member

Choose a reason for hiding this comment

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

I think this is a good idea but I'd recommend doing it in a future PR. this is already quite a bit dense for revewing.

src/ReactDOM.re Outdated
@@ -652,7 +652,7 @@ type domProps = {
[@mel.optional]
acceptCharset: option(string),
[@mel.optional]
action: option(string), /* uri */
Copy link
Collaborator

Choose a reason for hiding this comment

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

This comment and the one in rei were useful imo. Or were they not accurate?

Copy link
Member

Choose a reason for hiding this comment

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

@davesnx wasn't there something about this field that bothered you a bit?

can you link us to the issue about this? I think something about action being a function for RSC or something?

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverting this commit.

action is problematic in React 19 since it overloads it. formAction and action in this PR they are sync (while in React can be async and have different behaviour), if users want to use it they would need to use createElement + cloneElement or one of those tricks.

src/React.re Show resolved Hide resolved
"useOptimistic";

[@mel.module "react"]
external useTransitionAsync:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it expected that there will be another external for useTransition? Otherwise, why the Async suffix?

Copy link
Member Author

Choose a reason for hiding this comment

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

useTransition is sync (and stable since R18) and this is Experimental.useTransitionAsync

@@ -2,7 +2,7 @@ type undefined = Js.undefined(unit);

let undefined: undefined = Js.Undefined.empty;

[@mel.module "react-dom/test-utils"]
[@mel.module "react"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this is not gonna be in react-dom anymore, I find it confusing to keep it in ReactDOMTestUtils. Should we move it to React for consistency / principle of less surprise?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point, I should probably keep both and add a deprecation

Copy link
Collaborator

Choose a reason for hiding this comment

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

idk if deprecation is necessary. A ton of other things are breaking upstream for React 19, so maybe we can just move it directly.

Copy link
Member

Choose a reason for hiding this comment

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

agree this should be moved to react.re if it's coming from require('react')

Copy link
Member Author

Choose a reason for hiding this comment

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

There are a few "react" in ReactDOM module. I might probably propose to have this as a strict rule and align those as well in a future PR

davesnx and others added 3 commits November 20, 2024 14:19
Co-authored-by: Javier Chávarri <[email protected]>
Co-authored-by: Javier Chávarri <[email protected]>
Co-authored-by: Javier Chávarri <[email protected]>

/* https://react.dev/reference/react/useOptimistic */
[@mel.module "react"]
external useOptimistic:
Copy link
Member

Choose a reason for hiding this comment

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

can we @mel.uncurry some of these callbacks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I don't want to push code to this branch directly, done here: #867

/* https://react.dev/reference/react/useActionState */
[@mel.module "react"]
external useActionState:
(('state, FormData.t) => unit, 'state, ~permalink: bool=?) =>
Copy link
Member

Choose a reason for hiding this comment

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

should we add a unit argument at the end?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since I don't want to push code to this branch directly, done here: #867

@anmonteiro
Copy link
Member

@davesnx this is a big PR and there are some big changes. do you have a plan in your head or a link to something you've written about how to land these effectively in reason-react?

more specifically about this PR, how do you want to move forward and what's missing?

@davesnx
Copy link
Member Author

davesnx commented Nov 25, 2024

I was planning on using the branch "19" as a release candidate branch for all PRs depending on it, test it different repos (ahrefs included) and, if everything is good then do the opam release

* '19' of github.com:/reasonml/reason-react:
  Update src/React.re
  Update src/React.re
  Update src/React.re
* Install melange-testing-library

* Install melange-testing-library npm deps

* Vendor melange-testing-library

* Fix Form__test with RTL

* Start migrating Hooks__test

* Remove dependency

* Remove unused code from Form__test

* Add a jest-devtoolsgs

* Add a jest-devtools

* Migrate Hooks and Form into RTL

* Add demo to manually test easily

* Use Uncurried for tests

* Migrate all React__test

* Force install since we are dealing with R19

* Snapshot with lower {}

* Remove jest from demo/dune

* Add comment on install --force

* Bind React.act and React.actAsync

* Bind React.act and React.actAsync

* Use React.act as async version only

* Test react.act and react.actasync

* Fix hola test :(
* Install melange-testing-library

* Install melange-testing-library npm deps

* Vendor melange-testing-library

* Fix Form__test with RTL

* Start migrating Hooks__test

* Remove dependency

* Remove unused code from Form__test

* Add a jest-devtoolsgs

* Add a jest-devtools

* Migrate Hooks and Form into RTL

* Add demo to manually test easily

* Use Uncurried for tests

* Migrate all React__test

* Force install since we are dealing with R19

* Snapshot with lower {}

* Enable ref in ppx

* Add jest test for ref

* Remove jest from demo/dune

* Add comment on install --force

* Improve test from checking ref

* Bind React.act and React.actAsync

* Bind React.act and React.actAsync
@davesnx davesnx changed the title Support for React 19 new APIs Support for React 19 Nov 25, 2024
* 'main' of github.com:/reasonml/reason-react:
  Remove raise annotations and fix locations on errors (#863)
  ppx: support "custom children" in uppercase components without having to wrap in array literal (#823)
* '19' of github.com:/reasonml/reason-react:
  Enable ref as valid prop (#862)
  Replace react dom's testing library with ReactTestingLibrary (#859)
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.

4 participants