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

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

Open
AnderUstarroz opened this issue May 8, 2023 · 18 comments

Comments

@AnderUstarroz
Copy link

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))

@AnderUstarroz AnderUstarroz changed the title Add support for Node 18 (as easy as adding startTransition() on each setState) Add support for React & Node 18 (as easy as adding startTransition() on each setState) May 8, 2023
@multivoltage
Copy link

do you think someone supports this library?

@diasbruno
Copy link
Collaborator

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

@multivoltage
Copy link

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

@diasbruno
Copy link
Collaborator

@multivoltage Would you like to make a contribution?

@diasbruno
Copy link
Collaborator

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

Probably, something like this:

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

will trigger this behavior.

@diasbruno
Copy link
Collaborator

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
Copy link

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

@multivoltage
Copy link

@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
Copy link
Collaborator

diasbruno commented Feb 15, 2024

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
Copy link

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

@diasbruno
Copy link
Collaborator

It's already a peer dependency.

@multivoltage
Copy link

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

  • node 18/20
  • vite js

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

@doeg
Copy link

doeg commented Feb 16, 2024

@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
Copy link

multivoltage commented Feb 16, 2024

@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:

  • projects with react < 18 will use react-modal 3.x
  • projects with react >= 18 will use react-modal 4.x

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
Copy link

doeg commented Feb 16, 2024

@multivoltage I opened a separate issue here to address which versions of React should be supported: #1041

@doeg
Copy link

doeg commented Feb 16, 2024

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: facebook/react#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 #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 #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
Copy link

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

@diasbruno
Copy link
Collaborator

Bom trabalho, @doeg. 👍

Some notes:

  • The content of the modal is always rendered - <ExampleComponent />,
    even though it's not attached to the tree yet (isOpen starts false)
    (probably this explains why we need to add Suspense (?), but why the fallback <div /> is ok?)
  • Why adding startTransition work for @AnderUstarroz? Maybe the project already uses suspense for this?

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants