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

Router Context is Swallowed #25

Closed
dleavitt opened this issue Jun 24, 2017 · 12 comments
Closed

Router Context is Swallowed #25

dleavitt opened this issue Jun 24, 2017 · 12 comments

Comments

@dleavitt
Copy link

I am here to resurrect #19 and #16, for which I apologize.

I believe the current state of things is that mobx (and mobx-react-router) don't really work out of the box with react-router v4.

Cause:
observer components use a customized shouldComponentUpdate that pays attention to the mobx context but not the context that react-router needs.

Here's a repro:

Current workaround:
Users of this lib should wrap withRouter around every observer in their app.

Solutions:
Though this isn't mobx-react-router's fault (remix-run/react-router#4781, etc, etc) it does appear that this library owns the space at the intersection of mobx and react-router and is therefore in the best position to bundle a fix or workaround.

Short-term solution: This should be called out prominently in the readme. It's relevant to ~100% of people who use this lib (I think?)

Longer-term solutions: A fix or workaround should be integrated into this lib if possible. I think it is possible! Anything is possible, right? Here are some potential approaches:

  1. Whatever react-router-redux does. Assuming it has this problem solved, which I haven't verified.
  2. As part of syncHistoryWithStore, somehow turn the history attributes that react-router uses into observables. If that's a thing.
  3. Add a function/decorator that takes the context react-router needs out of the routing store and puts it into the routing context of the child component. You'd still need to apply this decorator to anything that uses a Route or NavLink, but at least you don't have to wrap every single observer in withRouter.
  4. Provide a router-friendly alternative to observer as part of this lib. This at least provides a solution out of the box and gets people like me to stop writing our own dubious one-offs.

Happy to submit a PR for the README or to look into options 3/4 if you agree with the above.

Thanks!
Daniel

@alisd23
Copy link
Collaborator

alisd23 commented Jun 26, 2017

Firstly the short-term solution: definitely. It seems many people are running in to this.

The long-term solution is a bit tricky. I would agree that looking into how (or if) react-router-redux solves this problem is the first step, then we can go from there.

I can try to find time in the next few days to look at react-router-redux properly, or if you're happy to - feel free to take a look through and record here what you find!

Thanks

@alisd23 alisd23 added the v4 label Jun 26, 2017
@alisd23
Copy link
Collaborator

alisd23 commented Jun 26, 2017

Done a bit of research, and I think react-routerredux` has not solved this problem either. It seems to have been left as a won't fix issue.

A relevant link: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md

@aldarund
Copy link

I have similar problem or even the same, not sure. I tried to wrap every observer with wihtRouter but it doesnt help.

export default function routeable(Child) {
  class RouteableObserver extends Component {
    render() {
      return React.createElement(Child, this.props)
    }
  }
  return withRouter(observer(RouteableObserver) )
}

Thats the decorator i tried to use.
The interesting thing is that it doesnt work only if i use Switch , if i just use Route all works fine. As soon as i surround my routes with Switch - nothing got updated except url until reload.
Is it the same problem? If so how to fix it? I need to use switch for 404 page...

@alisd23
Copy link
Collaborator

alisd23 commented Jul 11, 2017

If the component that renders the <Switch> component is wrapped by observer, then that is the component that needs withRouter.

@aldarund
Copy link

It is.. although i pinpointed problem to https://www.npmjs.com/package/lazy-route . If i use it with Switch it doesnt update even with withRouter, otherwise its fine.

@njleonzhang
Copy link

njleonzhang commented Aug 3, 2017

  const browserHistory = createBrowserHistory()
  const routerStore = new RouterStore()
  const history = syncHistoryWithStore(browserHistory, routerStore)
  history.subscribe((location, action) => console.log(location.pathname));

after use withRouter decorator, router(component render) can work, but mobx-react-router does not work. history does not change when path change.

sample project. one can help me out?


sorry, I am using BrowserRouter, should use Router.....
most of the doc and sample, use BrowserRouter, so I it is better to doc this as a important notice.

@clementdevosmc
Copy link

Does someone have a complete working example of this workaround?
Been playing with this for the past hours and still did not manage to make the whole thing work together...

@njleonzhang
Copy link

njleonzhang commented Oct 18, 2017

@clementdevosmc I have made a boilerplate. You can have a try.
https://github.com/njleonzhang/react-template

@alisd23
Copy link
Collaborator

alisd23 commented Oct 19, 2017

@clementdevosmc Notice the @withRouter wrapped wrapped round certain components. Worth reading this to understand the problem: https://github.com/ReactTraining/react-router/blob/master/packages/react-router/docs/guides/blocked-updates.md

@leikind
Copy link

leikind commented Feb 7, 2018

Correct me if I am wrong, but it seems like the short term solution proposed by @dleavitt to just add a couple of sentences to the readme to describe this problem has not been implemented, nothing has been added to the readme about this issue since June 2017, and the current readme keeps claiming that the package is a functional solution for react-router v4. Well, it is not.

@alisd23
Copy link
Collaborator

alisd23 commented Feb 8, 2018

Firstly: let's keep the sass to a minimum. There hasn't been activity on this issue for a while now and react-router-redux, the much more popular library, doesn't mention this at all in their README.

Secondly: I have now updated the README with some information about this issue, with relevant links.

The React guys are currently rewriting the context API, and should be released soon. This might affect (and hopefully fix) this issue, so we can revisit it then. Closing this for now.

@alisd23 alisd23 closed this as completed Feb 8, 2018
@leikind
Copy link

leikind commented Feb 9, 2018

👍

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

6 participants