reactjs / react-modal

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

Modal content failing to update properly with React Router #499

Open KyleJSummers opened 7 years ago

KyleJSummers commented 7 years ago

Summary:

I'm trying to do routing within a react-modal modal using React Router. I noticed an issue where route-based content requires two clicks of a link to display when rendered in a react-modal modal. To clarify what I mean by this, please check out the following example where I've isolated the pieces to a minimal to reproduce it.

https://codesandbox.io/s/yvnkj1zvzj

Steps to reproduce:

  1. Click the "Modal" link
  2. Click Link 1 (expected: it updates to show the content; actual: no update is observed)
  3. Click Link 1 again (content gets updated)

You can toggle between the two links, but it always seems to take an additional click after the URL updates for the content to update.

Expected behavior:

Content should update with the URL updates.

Link to example of issue:

https://codesandbox.io/s/yvnkj1zvzj

Additional notes:

I thought this might have to do something with react-modal pulling the components out of the flow, but I'm not sure. I tried specifying the prop for a parent container that was within the router subtree but that didn't seem to help. I wonder if it might be something with the use of ReactDOM.unstable_renderSubtreeIntoContainer with react-modal, but I'm really not sure.

Is routing within a react-modal modal not a use case that is readily supported? The example I prepared works if the modal is replaced with a simple wrapper component, so I'm quite confident the issue is somewhere within react-modal.

diasbruno commented 7 years ago

Hi @KyleJSummers, we need to investigate this. An workaround for this is to wrapper the modal in a stateful way, this should guarantee that the modal will rendered.

diasbruno commented 7 years ago

An curious behavior is:

Doing this will always require 2 clicks.

KyleJSummers commented 7 years ago

@diasbruno thanks. it seems to be one render pass behind, essentially.

diasbruno commented 7 years ago

@KyleJSummers You can check if there is something in componentWillUpdate() or componentWillReceiveProps preventing from update.

fabiulous commented 7 years ago

I have the same issue! Here's the description in stackoverflow: https://stackoverflow.com/questions/46338169/react-router-only-updating-after-second-click/46338565

At first I though it was something with my components and containers, then due to react-router, but finally I tracked it down to react-modal.

diasbruno commented 7 years ago

@shizpi @KyleJSummers If using react-router@4.2.0, you can use Switch.

<Switch location={props.location}>
   <Route path="/modal/1" component={() => <p>Content 1</p>} />
   <Route path="/modal/2" component={() => <p>Content 2</p>} />
</Switch>

Other hacky way I've found (force use props.location on render):

<Route location={props.location} path="/modal/1" component={() => <p>Content 1</p>} />
<Route location={props.location} path="/modal/2" component={() => <p>Content 2</p>} />

Example

diasbruno commented 7 years ago

The long story...nested Routes can get old values from the parent context. The reason for this is that getChildContext() (actually a provider for the children elements) will be updated after the component lifecycle, so it is possible that when Route tries to call computeMatch(), the context is the old one...but in the render of Route it will be the 'next context'.

The Switch, and/or probably other react-router components, will put the next location in place for the next Route.

cc @timdorr @mjackson to confirm this or provide more information, when possible.

timdorr commented 7 years ago

This is a common problem, so we've documented it: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md

Here's a fix for that codesandbox: https://codesandbox.io/s/rjw8o8p4v4

diasbruno commented 7 years ago

@timdorr Thank you for the answer.

KyleJSummers commented 7 years ago

Thanks guys. I'm familiar with the blocked updates issue when using react-router, and the workaround shared of explicitly setting the location via the use of withRouter, but I thought maybe this was something other than the typical blocked updates issue.

I understand when using a pure component or connect that they can block updates because they optimize around changing props, but this felt like something that might make sense to handle differently/could be a different issue (i.e. I haven't seen the same issue with other modal implementations in combination with RRv4).

Is this addressable more generally within react-modal itself in a way that doesn't block context updates? Or is there some render optimization that such a change would hinder? I have looked at the code a bit and didn't see where the blocking would occur, but I might have missed it. I notice the use of the "unstable" React method to render into a subtree and thought maybe that could possibly have an effect; but haven't dived into it.

Thanks again for taking a look at this and for sharing your insights! Much appreciated.

diasbruno commented 7 years ago

@KyleJSummers react-modal will block one update (when the modal is closed and will remain closed).

diasbruno commented 7 years ago

Forget my previous comment. I was look at the modal side, which is where the bug is...this really seems to be an issue with ReactDOM.unstable_renderSubtreeIntoContainer as you pointed out, @KyleJSummers. I have tried to debug this, but this is a tough one.

For now, this issue will remain open and to fix this, @timdorr example will be the best solution.

For future, we can see if changing ReactDOM.unstable_renderSubtreeIntoContainer to the new api unstable_createPortal (react 16, see issue #484) can help with this.

timdorr commented 7 years ago

It's just createPortal now. They made it official in the last prerelease.