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

Export context to allow history access #6365

Closed
TrySound opened this issue Oct 1, 2018 · 13 comments
Closed

Export context to allow history access #6365

TrySound opened this issue Oct 1, 2018 · 13 comments

Comments

@TrySound
Copy link
Contributor

TrySound commented Oct 1, 2018

Ref #6362 (comment)

Since "always render" behaviour will be removed from <Route children /> it would be good to provide a way to access context values from any part of app.

Here's a few ways and all requires to provide RouterContext.

RouterContext.Consumer

const Component = () => (
  <RouterContext.Consumer>
    {context => (
      <button onClick={() => {
        if (context) {
          context.history.push('/users')
        }
      }}>Open users</button>
    )}
  </RouterContext.Consumer>
)

Class component contextType

const Component = class extends React.Component {
  contextType = RouterContext;
  handler = () => {
    this.context.history.push('/users')
  }
}

context.unstable_read api

const Component = () => {
  const context = RouterContext.unstable_read();
  return (
    <button onClick={() => {
      if (context) {
        context.history.push('/users')
      }
    }}>Open users</button>
  )
}
@mjackson
Copy link
Member

mjackson commented Oct 1, 2018

I think you may be confusing 2 ideas. I'm proposing that we stop rendering something with a <Route> when it doesn't match. But it's still possible to create a <Route> that always matches. You can just use path="*" or no path at all:

<Route path="*" children={({ history }) => (
  <button onClick={() => history.push('/users')}>Go</button>
</Route>

// or even

<Route children={({ history }) => (
  <button onClick={() => history.push('/users')}>Go</button>
</Route>

In fact, we use a <Route> with no path in the implementation of withRouter, so you can use that too if you prefer to use a higher-order component instead.

@mjackson
Copy link
Member

mjackson commented Oct 1, 2018

BTW I'm a pretty strong -1 about exposing context as public API. That's our internal API. Our public API is <Route> and withRouter. You can get anything you need using them.

@timdorr
Copy link
Member

timdorr commented Oct 1, 2018

BTW, if it helps, I'm getting the strong sense that unstable_read is going away in favor of contextType (no "s"!): facebook/react#13728

@TrySound
Copy link
Contributor Author

TrySound commented Oct 1, 2018

@timdorr I use many context in my app. contextType allows to achieve only one. This sucks. unstable_read is pretty useful for translations.

import { t } from '../locale.js';
// localeContext.unstable_read().t under the hook
export const Component = () => (
  t('user')
)

@TrySound
Copy link
Contributor Author

TrySound commented Oct 1, 2018

@timdorr unstable_read is used for suspended queries. I don't think it will be removed in favour of contextType.
facebook/react#13337 (comment)

@jaredpalmer
Copy link
Contributor

@mjackson AFAIK exporting context is going to be normal/expected with what's coming. Formik is going to do the same.

@timdorr
Copy link
Member

timdorr commented Oct 1, 2018

Just a reminder: You will always be able to get to it via a direct import (import RouterContext from 'react-router/es/RouterContext'). So if you want to do some wonky stuff, you're free to aim the gun at your foot. This is just about making it a top-level export, which I agree with @mjackson isn't really necessary.

@mjackson
Copy link
Member

mjackson commented Oct 1, 2018

@jaredpalmer It just seems like context shouldn't be public. It's just a communication channel that we use to communicate between our components since we can't pass props down at every level.

@pshrmn
Copy link
Contributor

pshrmn commented Oct 1, 2018

I think the goal should be to eventually drop withRouter() and only use RouterContext.

Edit: To elaborate, I think of a context consumer (read from) as being public, while a provider (write to) is private. A <Route> is a bit unusual since it is both a consumer and a provider. A pathless <Route> (outside of a <Switch>) is only being used as a consumer, so it is just adding complexity when a user really just wants to read from the context.

Exposing the consumer will also let users use whatever future context optimizations for class components there are (Context.read(), static contextType, etc.). The current pattern of using a wrapper around classes that need access to the context in lifecycle methods is tedious at best.

@JeffBaumgardt
Copy link

JeffBaumgardt commented Oct 2, 2018

While I agree with @mjackson that we shouldn't access the private context, I have a argument to why context would be better.

As we know withRouter just wraps your component in a path less route giving your component access to all the props, mainly match and history in my use case.

The downside to withRouter and the HOC is that the react dev tools show a bunch of routes everywhere.

Getting access to the context Context.Consumer? We would be able to pluck off the props we want without the HOC.

Am I totally off here or is there a secret way to gain access to router props without wrapping your component in a route.

@mjackson
Copy link
Member

mjackson commented Oct 2, 2018

The fact that we use context under the hood is just an implementation detail!

Take the 4.4 release, for example. If context were part of our public API, changing from the old context to the new one would have been a breaking change, and we would've had a major version bump. But since it's not, we are free to refactor and move things around while still providing the same high-level API. That's the beautiful part about us not exposing context.

Also, what if the context implementation changes again (and it could)? If we expose it as public API, we will have the same problem.

@timdorr
Copy link
Member

timdorr commented Oct 2, 2018

Just to address the examples in the OP:

For the first example, <Route> is basically the same as the Consumer. So, this is equivalent:

const Component = () => (
  <Route>
    {({ history }) => (
      <button onClick={() => {
        if (history) {
          history.push('/users')
        }
      }}>Open users</button>
    )}
  </Route>
)

For the second example and in preparation of facebook/react#13728 landing, we could set a withRouter'ed component's contextType to support this.context usage without the side effects. But we would have this.props available, which would be a more ideal option. I'd rewrite this as:

class Component extends React.Component {
  handler = () => {
    this.props.history.push('/users')
  }
}
export default withRouter(Component)

And for the last example, I'm not sure if unstable_read is going to survive much longer now that contextType exists.

The React team definitely wants to discourage context abuse usage, so reliance on it as an official API for a library would seem to be counter to those goals. I'm aligned with @mjackson's feelings on the subject.

@mjackson
Copy link
Member

mjackson commented Oct 9, 2018

I'd say we can go ahead and close this for now since neither I nor @timdorr see any reason why you should need direct access to context. Happy to reopen if this changes in the future.

@mjackson mjackson closed this as completed Oct 9, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Dec 8, 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

6 participants