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

React 19 #607

Open
laurentpayot opened this issue Jun 3, 2024 · 9 comments
Open

React 19 #607

laurentpayot opened this issue Jun 3, 2024 · 9 comments

Comments

@laurentpayot
Copy link

Hi, React 19 RC has been released a few weeks ago, with some breaking changes.

How will the soon to be released React 19 impact Feliz? Is React 19 compatibility planned?

@MangelMaxime
Copy link
Contributor

We should indeed add support for React 19, however I still can't find information on what the expected JavaScript is.

All the documentation, I find are using JSX syntax but for Feliz we need to output result of the JSX compilation.

Currently this is what we are doing:

If in JSX you write

const MyButton = () =>  <button />

the JavaScript version of that was:

const MyButton = () => /*#__PURE__*/React.createElement("button", null);

The equivalent in Feliz is

[<ReactComponent>]
let MyButtion () =
	Html.button []

which compiles to

import { createElement } from "react";
import React from "react";

export function MyButtion() {
    return createElement("button", {});
}

Because of that I don't know yet if adding support for React 19, will be a breaking change or not.

It is also important to note that the code currently generated by Feliz is not even yet up to the latest version of what React does.

Since React 18 (I think), the recommended generated JavaScript is:

import * as JsxRuntime from "react/jsx-runtime";

var MyButton = JsxRuntime.jsx("button", null);

This is for example, the reason why when using Fast Refresh with Vite people need to set jsxRuntime: 'classic' for it to be working.

@Zaid-Ajaj
Copy link
Owner

Looking at the changes in React 19, nothing strikes me as breaking from Feliz point of view. That said, current Feliz version defines its react dependency (via a transitive reference to Fable.ReactDom.Types) as >= 18.x && < 19 so a breaking change is not out of the question if you happen to upgrade React to v19

@MangelMaxime
Copy link
Contributor

I think the easiest way to see if something is broken or not would be for someone to upgrade to React 19 and we will see if React complains or something breaks :)

@rasheedaboud
Copy link

I updated Feliz to with react 19 bits and the new compiler and it seems to work fine so far...
image

@tw0po1nt
Copy link

So I ran into an issue with this recently, actually.

My blog site - https://github.com/tw0po1nt/twopoint-website - uses Astro, and I recently tried integrating Fable and Feliz to write some custom React components in F#.

In setting up, I used the latest React that npm installed - v19 in my case - and for a simple component, I got multiple of the following error:

Screenshot_2024-12-18_at_2 17 54_PM

This is the component:

module Test

open Feliz

type Components =
  [<ReactComponent(exportDefault=true)>]
  static member Test(config: string, ?key: string) =
    Html.section [
      prop.className "relative not-prose scroll-mt-[72px]"
      prop.children [
        Html.div [
          prop.className "intersect-once motion-safe:md:intersect:animate-fade motion-safe:md:opacity-0 intersect-quarter mx-auto intercept-no-queue px-4 relative lg:py-20 md:px-6 md:py-16 py-12 text-default max-w-7xl"
          prop.children [
            Html.div [
              prop.className "mb-8 md:mb-12 md:mx-auto text-center max-w-3xl"
              prop.children [
                Html.h2 [
                  prop.className "font-bold font-heading leading-tighter tracking-tighter text-heading text-3xl md:text-4xl"
                  prop.text "[Title]"
                ]

                Html.p [
                  prop.className "text-muted text-xl mt-4"
                  prop.text "Lorem ipsum odor amet, consectetuer adipiscing elit. Mus tellus proin gravida litora consectetur iaculis conubia scelerisque. Hendrerit aenean massa tincidunt torquent tellus laoreet fusce habitasse."
                ]              
              ]
            ]
          ]
        ]
      ]
    ]

Bumping down to v18, this issue went away.

I'd gladly try to make a contribution to help support React 19 by fixing this issue (and potentially others, if they arise). Where should I start? 🙂

@MangelMaxime
Copy link
Contributor

The first thing would be to have a look at the generated code by standard tool when transforming JSX to check if there has been a meaningful change between React 18 and 19.

Second, would be to make a reproduction example as small as possible so you can more easily inspect the generated code.

  • Remove classes
  • Make the number of nested HTML element as small as possible

Check if the generated code does use one of these API:

Feliz/Feliz/Interop.fs

Lines 19 to 22 in 6841c9b

let inline reactElementWithChild (name: string) (child: 'a) =
reactElement name (createObj [ "children" ==> ResizeArray [| child |] ])
let inline reactElementWithChildren (name: string) (children: #seq<ReactElement>) =
reactElement name (createObj [ "children" ==> reactApi.Children.toArray (Array.ofSeq children) ])

If that's the case, look at React the result of applying these functions between React 18 / 19 to see if they changed something. If I remember correctly, in the past using reactApi.Children.toArray made React add a key which consist of an index (1, 2, etc.) which is the less useful key ever but is actually the default behaviour that React used for optimisation.

@lukaszkrzywizna
Copy link
Contributor

lukaszkrzywizna commented Dec 19, 2024

My production app also encounters issues with key warnings. The problem lies in how React handles components processed with React.Children.toArray. Use the following code snippet, and you will notice a difference in the console output depending on whether version 18 or 19 is used:

function Test() { 
  return <div></div>;
}
function App() {
  return (
    <>
      {React.Children.toArray([
        <Test/>
      ])}
    </>
  )
}

I wonder if this might be a React bug since components receive keys, but not in a direct way.

Regarding Switching to jsxRuntime

I considered switching to jsxRuntime when I first encountered a problem where my component's state was lost because I forgot to assign explicit keys to dynamically added children. In that case, Children.toArray() becomes problematic because, without it, React would typically warn you about missing keys.

I decided to investigate how React apps normally distinguish between cases where explicit keys are necessary. It turns out that the JSX compiler differentiates based on how the children are provided:

const Static = () => {
  return 
    <div>
      <span/>
      <span/>
    </div>; 
}

const Dynamic = () = {
  <div>
    {[1,2,3].map((_) => (<div/>))}
  </div>; 
}

The compiled output is as follows:

const Static = () => {
  return;
  /*#__PURE__*/(0, _jsxRuntime.jsxs)("div", {
    children: [/*#__PURE__*/(0, _jsxRuntime.jsx)("span", {}), /*#__PURE__*/(0, _jsxRuntime.jsx)("span", {})]
  });
};
const Dynamic = () => {
  /*#__PURE__*/(0, _jsxRuntime.jsx)("div", {
    children: [1, 2, 3].map(_ => /*#__PURE__*/(0, _jsxRuntime.jsx)("div", {}))
  });
};

The key difference lies in the use of different functions (jsx vs jsxs). If we examine React's source code, we see that the distinction between these functions is a flag enabling or disabling child key validation:

// While `jsxDEV` should never be called when running in production, we do
// support `jsx` and `jsxs` when running in development. This supports the case
// where a third-party dependency ships code that was compiled for production;
// we want to still provide warnings in development.
//
// So these functions are the _dev_ implementations of the _production_
// API signatures.
//
// Since these functions are dev-only, it's ok to add an indirection here. They
// only exist to provide different versions of `isStaticChildren`. (We shouldn't
// use this pattern for the prod versions, though, because it will add an call
// frame.)
export function jsxProdSignatureRunningInDevWithDynamicChildren(
  type,
  config,
  maybeKey,
  source,
  self,
) {
  if (__DEV__) {
    const isStaticChildren = false;
    return jsxDEVImpl(
      type,
      config,
      maybeKey,
      isStaticChildren,
      source,
      self,
      __DEV__ && enableOwnerStacks ? Error('react-stack-top-frame') : undefined,
      __DEV__ && enableOwnerStacks ? createTask(getTaskName(type)) : undefined,
    );
  }
}

export function jsxProdSignatureRunningInDevWithStaticChildren(
  type,
  config,
  maybeKey,
  source,
  self,
) {
  if (__DEV__) {
    const isStaticChildren = true;
    return jsxDEVImpl(
      type,
      config,
      maybeKey,
      isStaticChildren,
      source,
      self,
      __DEV__ && enableOwnerStacks ? Error('react-stack-top-frame') : undefined,
      __DEV__ && enableOwnerStacks ? createTask(getTaskName(type)) : undefined,
    );
  }
}

const jsx: any = __DEV__
  ? jsxProdSignatureRunningInDevWithDynamicChildren
  : jsxProd;
// we may want to special case jsxs internally to take advantage of static children.
// for now we can ship identical prod functions
const jsxs: any = __DEV__
  ? jsxProdSignatureRunningInDevWithStaticChildren
  : jsxProd;

Suggestion for Fable

I suggest replicating this behavior in Fable (or a Fable plugin) by analyzing how the children prop is constructed or use jsxs everywhere to get rid of an warning completely.

Additional Note

Using jsxRuntime functions can be tricky because they do not accept key as a standard prop they expect key as a seperated argument:

<div key={"my-key"} className={"class"}/>

gives:

/*#__PURE__*/(0, _jsxRuntime.jsx)("div", {
  className: "class"
}, "my-key");

Provide key as a prop works, but causes displaying an error:
image

My guess is that we would need to detect when a dynamically provided key (via the props collection) is present and explicitly insert it into the jsx function.

@laurentpayot
Copy link
Author

@lukaszkrzywizna I’m just wondering, why jsxRuntime instead of the much more battle-tested Preact? Preact is also a 3Kb drop-in replacement for React and works perfectly for me with Feliz (and Vite).

@lukaszkrzywizna
Copy link
Contributor

@laurentpayot jsxRuntime is a part of React. The question is more about which framework to use. My production apps heavily rely on React-based libraries like react-query, shadcn/ui, and others. Not all of them are compatible with preact.

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

No branches or pull requests

6 participants