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

HMR does not work in modal #7248

Open
1 task done
telcy opened this issue Aug 24, 2023 · 8 comments
Open
1 task done

HMR does not work in modal #7248

telcy opened this issue Aug 24, 2023 · 8 comments
Assignees
Labels
bug:unverified feat:dx Issues related to the developer experience package:dev

Comments

@telcy
Copy link

telcy commented Aug 24, 2023

What version of Remix are you using?

1.19.3

Are all your remix dependencies & dev-dependencies using the same version?

  • Yes

Steps to Reproduce

  1. clone https://github.com/telcy/remix-hmr-modal-issue
  2. install dependencies
  3. click "simple dialog" button
  4. change modal text to trigger hmr

Expected Behavior

Modal should stay open and text should get replaced.

Actual Behavior

Modal closes

@brophdawg11
Copy link
Contributor

I'm not familiar with mui-modal-provider but I'm not sure if this is a problem with Remix or not. Looking at it briefly, useModal is destroying the modal in an effect:

  useEffect(
    () => () => {
      if (!disableAutoDestroy) {
        destroy(id.current);
      }
    },
    [disableAutoDestroy, destroy]
  );

destroy there comes from their own internal useContext(ModalContext), so it seems as if it's identity is not stable.

I tested the useModal({ disableAutoDestroy: true }) option and that keeps the modal open during HMR - but I'm unsure of the impacts of keeping that option enabled in production use-cases.

@pcattori unless you have any specific ideas for where Remix HMR might be causing this I'm thinking this may need to be investigated on the MUI side?

@brophdawg11 brophdawg11 added package:dev feat:dx Issues related to the developer experience labels Aug 25, 2023
@telcy
Copy link
Author

telcy commented Aug 25, 2023

I thought I might add that Vite and Next do not have this issue with the exact same modal setup. Unsure if it is a bug or just the nature how Remix's HMR works.

Edit:
https://github.com/telcy/next-simple-dialog
https://github.com/telcy/vite-simple-dialog

@dantxal
Copy link

dantxal commented Aug 25, 2023

I have a similar problem with modals, where HMR simply doesn't work (it refreshes the page).

Just by having an OverlayProvider component from 'react-aria' as a parent component for the Outlet in root.tsx, is enough to stop HMR from working. Even if there is no modal using that provider.

I will take a look at the implementation of the OverlayProvider myself, since I am still unsure whether that is a problem with that library or remix.

EDIT: Here is the repro: https://github.com/dantxal/repro-issue-7248-remix
Simply clone the repo and follow the instructions on Readme file to reproduce.

@dmarkow
Copy link
Contributor

dmarkow commented Aug 25, 2023

Same thing happens for me -- it's most obvious in modals (which are always triggered by a setState call), but really all state gets blown away on any HMR update in my app. For me it was DndProvider from react-dnd, which wraps my whole app. At a glance it seems to be a basic wrapper around a react context, but something about it doesn't like HMR. Sounds like a similar problem with these other providers.

@telcy
Copy link
Author

telcy commented Sep 21, 2023

@dantxal @dmarkow
As you said, it's losing all state on HMR updates. Have you found a solution for this issue?

@pcattori
Copy link
Contributor

pcattori commented Sep 21, 2023

I wrote up some limitations of React Fast Refresh in the docs. Could be that things like DndProvider are class components? 🤷

But for issues that aren't reproducible in Vite nor Next, then the issue is most likely Remix-related, not a React Fast Refresh limitation.

@dmarkow
Copy link
Contributor

dmarkow commented Sep 22, 2023

@pcattori I know at least for react-dnd, it doesn't use classes. I remember seeing something similar in vite, where contexts were breaking HMR. They did something to the react-refresh code to deal with this.

@telcy
Copy link
Author

telcy commented Sep 22, 2023

I took a look into mui-modal-provider and it does not use classes either.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug:unverified feat:dx Issues related to the developer experience package:dev
Projects
None yet
Development

No branches or pull requests

5 participants