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

withRouter: Directly use RouterContext instead of Route. #6685

Merged
merged 1 commit into from Apr 3, 2019
Merged

withRouter: Directly use RouterContext instead of Route. #6685

merged 1 commit into from Apr 3, 2019

Conversation

StringEpsilon
Copy link
Contributor

Currently, withRouter(Foo) would render with 5 levels:

<withRouter(Foo)>
  <Route>
    <ContextConsumer>
      <ContextProvider>
        <Foo/>
      </ContextProvider>
    </ContextConsumer>
  </Route>
</withRouter(Foo)>

With this PR, it's only 3 levels:

<withRouter(Foo)>
  <ContextConsumer>
    <Foo/>
  </ContextConsumer>
</withRouter(Foo)>

I also took the opportunity to clarify the warning. Previously, using withRouter(Foo) would have yielded "You should not use <Route> outside a <Router>". I've ran into that warning a couple of times myself and it's particularly unhelpful.

This reduces the nesting introduced by withRouter() quite a bit and makes for an easier warning in case it's used outside <Router/>
@timdorr
Copy link
Member

timdorr commented Apr 3, 2019

Hmm, any sort of unintended behavior changes here? That's the only problem I can think of, but I can't come up with an example.

@pshrmn
Copy link
Contributor

pshrmn commented Apr 3, 2019

It should be fine. The only reason I can think of that removing the Route would change the behavior would be if it was possible to pass props to the Route to effect the output. Absent that, the Route is the component equivalent of an identity function.

@StringEpsilon
Copy link
Contributor Author

StringEpsilon commented Apr 3, 2019

Seems like withRouter used to operate directly on context early on. Here is the commit that changed it to a <Route/>.

Also, Route doesn't do anything with the context when there are no props. It just consumes the context then overrides the context with the same value. It basically does this:

<RouterContext.Consumer>
  {context => {
    const props = { ...context };

    return (
      <RouterContext.Provider value={props}>
        {children }
      </RouterContext.Provider>
    );
  }}
</RouterContext.Consumer>

Edit: I guess it might cause breakage, if someone relies on withRouter() rendering that <Route/>. But I can't think of any reason why (or how) someone would do that.

@MeiKatz
Copy link
Contributor

MeiKatz commented Apr 3, 2019

Since the referenced commit is two years old and therefore from a time when there was no official React Context API, I think with the new tools the problems from then are gone. So your current solution should work as expected. Maybe it breaks some tests but not the code as it.

@timdorr
Copy link
Member

timdorr commented Apr 3, 2019

Yeah, unless someone can come up with some big objection, I think this is good. I'll merge it in. If anyone wants to undo it, revert this PR 😄

@timdorr timdorr merged commit 10d78bb into remix-run:master Apr 3, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jun 2, 2019
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.

None yet

4 participants