Skip to content

Conversation

benjaminhoffman
Copy link
Contributor

I read this doc like four times until I decided to click on the breadcrumbs for other guides. Lo and behold, I found and solved my problem within 30 seconds. This PR helps others avoid my situation because it indicates that they should also read the other Redux guide if they are experiencing blocked updates.

I read this doc like four times until I decided to click on the breadcrumbs for other guides.  Lo and behold, I found and solved my problem within 30 seconds.  This PR helps others avoid my situation because it indicates that they should also read the other Redux guide if they are experiencing blocked updates.
@pshrmn
Copy link
Contributor

pshrmn commented Sep 22, 2017

I'm not sure this is really necessary. Redux is already mentioned in the third-party code section.

Should the solution in that guide be more explicit? I guess a tl;dr could be added. When I wrote that, I wanted to discourage overuse of withRouter because it isn't necessary, but it is easy so it seems to have become the de facto solution.

@benjaminhoffman
Copy link
Contributor Author

Thx for immediate reply! I think it should at least be linked to somewhere... it was literally a 30 second fix vs. about an hour yesterday.

Happy to update this PR... I can add it to the third-party code section. Lemme know what you had in mind.

You recommend the passing of location everywhere vs. simply using withRouter? That seems counterintuitive but I'm likely missing some context here? The former solution seemed cumbersome and it didn't quite fit my use case.

@pshrmn
Copy link
Contributor

pshrmn commented Sep 22, 2017

@benjaminhoffman the fix for blocked updates is just passing a prop that changes when the location changes. When you use withRouter, you're adding additional components into the element tree (the Route that that renders). If you already have access to the location when you , it seems like a waste to render the extra components when you can just pass a prop to achieve the same thing. The worst is when I see a component already rendered by a <Route> being wrapped with withRouter:

const MyComponent = withRouter(connect(...)(AComponent));

<Route path='/somewhere' component={MyComponent} />
/*
 * <Route path='/somewhere>
 *   <withRouter()>
 *     <Route>
 *       <connect()>
 *         <AComponent>
*/

withRouter certainly can be useful when you don't have ready access to the location, and I realize that I'm fighting a losing battle championing props over withRouter, but it just feels wrong to me to have "wrap it in a withRouter" as the go to solution.

That isn't meant to be a criticism of this PR. The docs aren't doing their job if they aren't providing a clear solution, and the current blocked updates guide takes its time getting to the answer. I'm fine with the link, although I think that posting something like this at the top of this guide might be more useful:

Quick Answer

If you are running into this issue while using a higher-order component like connect (from react-redux) or observer (from Mobx), you can just wrap that component in a withRouter to remove the blocked updates.

// redux before
const MyConnectedComponent = connect(mapStateToProps)(MyComponent)
// redux after
const MyConnectedComponent = withRouter(connect(mapStateToProps)(MyComponent))

// mobx before
const MyConnectedComponent = observer(MyComponent)
// mobx after
const MyConnectedComponent = withRouter(observer(MyComponent))

This is not the most efficient solution, but will prevent the blocked updates. If you are interested in learning why this is happening, the rest of this guide will cover that.

@benjaminhoffman
Copy link
Contributor Author

@pshrmn ah! Makes sense. I updated the PR with more context -- added your description + two links (one to this thread and the other to redux guide).

Although you mentioned to put this at the top, I would hate to have people read just this section then move on. It's probably better they read the page to get educated on the issue and once they hit the solution section, they'll see this discussion.

Thoughts? Happy to change it if you think that's better.

Copy link
Contributor

@pshrmn pshrmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As much as I want people to read the guide and understand the problem, I'm fine putting this at the top if it means that it will help more people. Your call, either one is fine with me.

If you do move it to the top, maybe just put the "this is not the most efficient" sentence in bold so that it stands out a little bit more and maybe convinces a few people to read the rest?

Other than that, I'm good with this. Thanks for the PR!

@benjaminhoffman
Copy link
Contributor Author

@pshrmn I tried to rework it in the intro but after trying a few copy updates, it just sounded weird / out of context given that it's a redux/mobx related issue. Although, I did bold a sentence in my most recent commit. Nonetheless, always happy to try again / make updates if you'd like!

Ready to merge? (I dont see a "merge" option on this PR).

@pshrmn
Copy link
Contributor

pshrmn commented Sep 24, 2017

@benjaminhoffman It looks good to me, but another collaborator needs to OK and merge this.

@timdorr
Copy link
Member

timdorr commented Oct 3, 2017

Sorry, let's merge this in!

@timdorr timdorr merged commit bcf8cb9 into remix-run:master Oct 3, 2017
@benjaminhoffman benjaminhoffman deleted the patch-1 branch October 3, 2017 23:28
@Sawtaytoes
Copy link

I might've found a different solution. I don't know if it's going to be the end-all fix, but what about this?
#5843 (comment)

@mystyle2006
Copy link

Thanks!

@lock lock bot locked as resolved and limited conversation to collaborators Jun 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants