reactjs / react-modal

Accessible modal dialog component for React
http://reactcommunity.org/react-modal
MIT License
7.35k stars 810 forks source link

Add support for React & Node 18 (as easy as adding startTransition() on each setState) #1019

Open AnderUstarroz opened 1 year ago

AnderUstarroz commented 1 year ago

Summary:

Current react modal doesn't work with latest version of React + Node v18

Steps to reproduce:

  1. Use react-modal with latest React's and Node v18

Expected behavior:

Should work normally

Example of issue:

Breaks with the following error message:

Screenshot 2023-05-08 at 15 42 54

Solution:

Simply add startTransition() wrapping each of the calls for setting the state. startTransition(() => setState("something))

multivoltage commented 8 months ago

do you think someone supports this library?

diasbruno commented 8 months ago

@AnderUstarroz It seems easy. Would you like to make a contribution?

multivoltage commented 8 months ago

@AnderUstarroz can you provide an eample? I installed correctly a basic example with react 18 yesterday

diasbruno commented 8 months ago

@multivoltage Would you like to make a contribution?

diasbruno commented 8 months ago

Maybe it's related to the weird <suspense /> component.

Probably, something like this:

<suspense fallback="...">
  <modal />
</suspense>

will trigger this behavior.

diasbruno commented 8 months ago

I'm guessing to fix this isse the solution is to wrap the setIsOpen() (how people call it). But it's application specific, so we don't break older reaect versions.

function toggleModal() {
    startTransition(() => {
      setIsOpen(!isOpen);      
    });
}
itsjesusmacias commented 7 months ago

Is it only necessary to set setIsOpen? I would like to be able to carry out this task to contribute to the project

multivoltage commented 7 months ago

@diasbruno yes but as I said before in my example all work great so... @itsjesusmacias yes but using start-transiction means a major release since startTransition is only react > 18

diasbruno commented 7 months ago

Thanks, @multivoltage. @itsjesusmacias Give it a try...I'm hoping something like this would help, otherwise we need to investigate further this issue.

As @multivoltage said, this feature it released on react 18+, so if we need to make a fix for it, we need to be careful to not break for older versions.

multivoltage commented 7 months ago

Maybe we can add "peerDependencies" react >=18 ?

diasbruno commented 7 months ago

It's already a peer dependency.

multivoltage commented 7 months ago

@AnderUstarroz please can you provide a demo? I tried a 2nd time to reproduce it but with my stack;

Using demo example on homepage here https://www.npmjs.com/package/react-modal seem to work without errors

doeg commented 7 months ago

@multivoltage with respect to your comment:

Maybe we can add "peerDependencies" react >=18 ?

To build on what you and @diasbruno have been saying, this would require all projects using react-modal to be on React v18+, no? This would generally be considered a breaking change requiring a new major version release as you pointed out above. :)

Given the popularity of this project, I don't think supporting only the latest version of React is a reasonable peer dependency even if there was a major release.

multivoltage commented 7 months ago

@doeg How I said before when there is a real demo project with bug I'll happy to investigate more :) but talking about your idea I think:

In my experience this is normal. When this happends the readme should include a row like: from react-modal 4 only react > 17 is supported. Migrate or keep going to use v3

People who install for some reason 4.x in a project with react 17 (or 16) will read a red warning in the console after npm i react-modal

doeg commented 7 months ago

@multivoltage I opened a separate issue here to address which versions of React should be supported: https://github.com/reactjs/react-modal/issues/1041

doeg commented 7 months ago

I've spent some time in CodeSandbox trying to reproduce the error noted above:

A component suspended while responding to synchronous input. This will cause the UI to be replaced with a loading indicator. To fix, updates that suspend should be wrapped with startTransition.

This issue in the React repo goes into some detail about the error: https://github.com/facebook/react/issues/25629

The simplest reproduction I've gotten so far is attempting to lazy load a component and render it within a modal without wrapping it in a Suspense block.

CodeSandbox link: https://codesandbox.io/p/sandbox/react-18-react-modal-suspense-error-vq79xg

import "./styles.css";
import ReactModal from "react-modal";
import { useState, lazy, Suspense } from "react";

const ExampleComponent = lazy(() => import("./ExampleComponent"));

export default function App() {
  const [showModal, setShowModal] = useState(false);

  return (
    <div className="App">
      <button onClick={() => setShowModal(true)}>Open Modal</button>

      <ReactModal isOpen={showModal} onRequestClose={() => setShowModal(false)}>
        {/* Uncomment the Suspense boundary below to resolve the error */}
        {/* <Suspense fallback={<div>Loading...</div>}> */}
        <ExampleComponent />
        {/* </Suspense> */}
      </ReactModal>
    </div>
  );
}

image

In this case, adding a Suspense boundary around the problematic component is enough to solve the issue:

image

I've yet to find a way to reproduce the A component suspended while responding to synchronous input error that points to the issue being within react-modal itself.

I've also yet to find anything that would suggest React Modal won't work with React v18, though as mentioned in https://github.com/reactjs/react-modal/pull/1040 I'm going to do some experimentation with React Modal + React v18 in our very complex codebase next week. :)

The only other potentially related issue with React Modal + Suspense could be https://github.com/reactjs/react-modal/issues/982.

@AnderUstarroz as others have mentioned, it would be super useful to get a reproducible example of what's producing this error for you. 🙇 Otherwise, @diasbruno, it seems like this one could perhaps be closed out as "not a bug"...?

multivoltage commented 7 months ago

Maybe README can be edited with some details to help developer in this use-case?

diasbruno commented 7 months ago

Bom trabalho, @doeg. 👍

Some notes:

@itsjesusmacias Feel free to jump in anytime. The more people, more ideas.