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

using the new context provider from react 16.3 breaks the router #6072

Closed
bogdansoare opened this issue Apr 8, 2018 · 18 comments
Closed

using the new context provider from react 16.3 breaks the router #6072

bogdansoare opened this issue Apr 8, 2018 · 18 comments

Comments

@bogdansoare
Copy link
Contributor

Version

4.2.2

Test Case

https://codesandbox.io/s/rjl08qoxlp

Steps to reproduce

Use a context provider inside the router component

Expected Behavior

Using the new context provider should not break the router.

Actual Behavior

Using the new context provider is breaking the router.

@pshrmn
Copy link
Contributor

pshrmn commented Apr 8, 2018

It looks like this is only an issue if the router and context provider elements are created in the same render call. It isn't immediately clear in my mind why this is the case.

// does not work
const One = () => (
  <Router>
    <Context.Provider>
      <Route />
    </Context.Provider>
  </Router>
);
ReactDOM.render(<One />, ...);

// does not work
const Two = () => (
  <Route />
);
ReactDOM.render((
  <Router>
    <Context.Provider>
      <Two />
    </Context.Provider>
  </Router>
), ...);

// works
const Three = () => (
  <Context.Provider>
    <Route />
  </Context.Provider>
);
ReactDOM.render((
  <Router>
    <Three />
  </Router>
), ...);

Edit Sandbox demo https://codesandbox.io/s/oo6xmjo936

@gaearon
Copy link
Contributor

gaearon commented Apr 8, 2018

Might be related? facebook/react#12551

@pshrmn
Copy link
Contributor

pshrmn commented Apr 8, 2018

Will track via the React issue. For the time being, anyone running into this issue should just render the router and context provider separately.

@dantman
Copy link

dantman commented Apr 10, 2018

I'm having an issue with Routes not updating when my hash/path changes. Which may be this issue.

However while I am using the new context system, I do not think this is an issue specific to the new context system. As I see the following in the script. The old context system was never reliable enough to expect that context would be updated, the moment you try to use PureRender/shouldComponentUpdate to make your application run efficiently the old context system falls apart. Everyone using the old context system was expected to only pass static data down through context, if dynamic data was desired the accepted pattern was to pass down some sort of static observer so the consumers could subscribe instead of expecting context to be updated.

  Router.prototype.getChildContext = function getChildContext() {
    return {
      router: _extends({}, this.context.router, {
        history: this.props.history,
        route: {
          location: this.props.history.location,
          match: this.state.match
        }
      })
    };
  };

I do not believe this is something that is React's job to fix, getChildContext is after all part of an experimental feature that was never considered stable and is now obsolete now that we have a new context system. IMHO in this case this is a bug that is react-router's responsibility, in this case the fix is to update to use the new context API. If you want support for React 16.2, there is a create-react-context polyfill library.

@timdorr
Copy link
Member

timdorr commented Apr 10, 2018

@dantman
Copy link

dantman commented Apr 10, 2018

@timdorr Sounds exactly like what I thought. This is a react-router bug, the update behaviour of the old context API should never have been relied on. react-router should have been passing a subscriber down or using a 3rd party context wrapper that does so. Not expecting the old context API to update data and writing a long explainer on how to workaround the issue.

I'm not going to pepper my codebase with workarounds because react-router is not using context correctly. I guess I'm just going to have to abandon react-router until it starts using the new context API.

@brianespinosa
Copy link

I'm not going to pepper my codebase with workarounds because react-router is not using context correctly.

Unfortunately this is what we had to do in our large application. We did not realize until too late in the refactor to the latest RR version that this was a problem. Luckily we were able to make a HOC that we swapped out all Link components with that uses redux to do this instead.

@remix-run remix-run deleted a comment from CyrusZei Apr 19, 2018
@remix-run remix-run deleted a comment from CyrusZei Apr 19, 2018
@renatonmendes
Copy link

@brianespinosa Can you please share with us how you´ve solved that ? I need to use the context provider with routing without success.

@brianespinosa
Copy link

@renatonmendes we had to create our own Webpack plugin that replaces our use of the react-router-dom package with a custom implementation where we store a location key in Redux. Once React Router is updated to support the proper use of the new context API, we simply remove our plugin and it goes back to using the real react-router-dom package as normal.

Past this explanation, unfortunately I cannot share any code with you from my current project.

@mrbinr
Copy link

mrbinr commented May 3, 2018

Hi,
I have modified first code from @bogdansoare and it seems to works now : https://codesandbox.io/s/xon5z2nk9z

@gaearon
Copy link
Contributor

gaearon commented May 24, 2018

Should be fixed in React 16.4.
https://reactjs.org/blog/2018/05/23/react-v-16-4.html

@szagi3891
Copy link

This problem still exists in my project (reacta version 16.4). It occurs in such a case:

        ReactDOM.render((
            <ApolloProvider client={client}>
                <BrowserRouter>
                    <AppWithRouter />
                </BrowserRouter>
            </ApolloProvider>
        ), root);

@gaearon
Copy link
Contributor

gaearon commented May 24, 2018

Can you provide a new small reproducing case? Ideally without any third party libs, just React.

@timdorr
Copy link
Member

timdorr commented Jun 6, 2018

This should be fixed in 16.4, so I'm closing this out.

@Martsyalis
Copy link

Martsyalis commented Jun 8, 2018

@szagi3891 did you add withRouter to your Context Component? That solved it for me with 16.4

@johannessteu
Copy link

@Martsyalis just like this?

import React from 'react';
import { withRouter } from 'react-router'

const AppContext = React.createContext();

export default withRouter(AppContext);

Does not work for me actually >.>

@szagi3891
Copy link

@Martsyalis
Yes, I used this function this way:

        const AppWithRouter = withRouter(() => <App />);

        ReactDOM.hydrate((
            <ApolloProvider client={client}>
                <BrowserRouter>
                    <AppWithRouter />
                </BrowserRouter>
            </ApolloProvider>
        ), root);

Then the react-router started to work properly.

@Martsyalis
Copy link

withRouter should be used on every component that relies on history or match props. So everything that listens to url.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 14, 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

No branches or pull requests