contextType: convenience API for reading context in a class #65
Conversation
import { Consumer } from "some-package";
class Button extends React.Component {
static contextType = Consumer;
}
class Button extends React.Component {
static contextType = context;
componentDidMount() {
this.prevContext = this.context;
}
componentDidUpdate() {
if (this.context !== this.prevContext) {
// ...
}
}
} |
|
ya this is awesome! supporting multiple contexts would be really nice as well, but i see how that would be harder. I would say that if it was feasible it may be worth doing, our apps are more and more looking like |
|
I'm in favor of not doing anything.
|
No, we discourage using Consumer for anything other than a render prop. It accidentally is the same object as context today but we’re going to start warning about this. I’ll write up a better explanation next week for why this makes sense after I get back from React Conf. But for now please export Context from libraries directly.
This
We have something in the works that helps with this. Will likely post an RFC next week.
Even if classes become less relevant we still plan to support them for the observable future. Speeding up the adoption of new context is a high priority which makes this worth the redundancy compromise to us.
I don’t see why, can you explain more? |
|
If I'm understanding correctly, you would no longer need to initialize Context outside the class with this change. Is that correct or is that considered an anti-pattern? |
|
|
||
| Arguably this could have been a part of the initial `React.createContext` proposal. We'd still, however, need a low-level API for function components or combining multiple contexts. So this couldn't be the only API. At the time `React.createContext` was proposed, we were hoping to be able to make more targeted optimizations to context propagation. However, as we worked on Suspense, we realized that we need the capability to read context without an explicit `Consumer` render prop anyway — otherwise accessing a Suspense cache is too cumbersome. Since this makes the original performance optimization justification weaker, and we might as well add a convenience API if we can't take advantage of the low-level API constraints. | ||
|
|
||
| This API proposal is intentionally limited and **doesn't** support multiple contexts directly. It is only a shortcut for the common case. If you need to read several contexts (or read it from function components), you can use the existing API. |
There was a problem hiding this comment.
So excited for this proposal! Can you explain why this limit is here?
There was a problem hiding this comment.
Static typing and performance reasons.
This change is just a shortcut for |
|
It's great, but! class Button extends React.Component {
static contextType = ThemeContext; // Put this here
render() {
const theme = this.context; // Then you can use this here
...
}
}Binding the context through static field always perceived as something alien in React. I hope there is less strange way to do this. E.g.: class Button extends ThemeContext.Component { // <---
render() {
const theme = this.context;
...
}
}or class Button extends React.Component {
context = React.consumeContext(ThemeContext); // don't know if it possible at all
render() {
const theme = this.context;
...
}
} |
|
@AlexGalays Okay got it. Still a big fan of the change |
|
@strayiker The API for multiple contexts would be much worse if you did it w/ inheritance. |
|
@mAAdhaTTah Totally agree. But I assume that this RFC for simplest case with single context. |
|
Sounds really cool! I'd also love to see support for multiple contexts if we can agree on an intuitive API. What about something like this? class Foo extends React.Component {
static contextTypes = {
theme: ThemeContext,
route: RouteContext
}
render() {
const theme = this.context.theme;
const route = this.context.route;
}
} |
|
@cangoektas |
I'm probably missing some context 😏 that you'll cover in your explanation. My understanding of the As far as actual design goes, |
It would cause us to allocate intermediate objects on every render. This is too expensive and brings back some of the problems with the legacy context API. Such as making it difficult to type statically. |
|
|
||
| # Alternatives | ||
|
|
||
| * Don't do anything (status quo). This significantly slows down the adoption of new context. |
There was a problem hiding this comment.
What are the specific problems with the new context that have slowed adoption, and that this solves? Accessing context in lifecycles?
There was a problem hiding this comment.
Accessing context in lifecycles and the need to forward refs to do that. It's awkward.
| # Alternatives | ||
|
|
||
| * Don't do anything (status quo). This significantly slows down the adoption of new context. | ||
| * Call the field something different, like `contextConsumerType`. |
There was a problem hiding this comment.
I wonder how many people will try and use PropTypes to define contextType thanks to contextTypes. Maybe it should use a name that is even further from contextTypes so that there's no risk of confusing them?
There was a problem hiding this comment.
We warn immediately if you pass something else.
|
|
|
It still is but also still unstable. Not an official API and we don't know yet if it will be official. This particular RFC doesn't include support for it, you'd need to still use |
There was a problem hiding this comment.
(I am reiterating, but since this is in the "final comment period", I wanted to throw an actual review out there)
While this API is intended to increase context adoption, it requires packages to expose their entire context to the user.
I would much rather have static contextType (or maybe static contextConsumer/static consumes) be required to be a context consumer. This would more closely mirror its use case and allow packages to keep parts of their context (i.e. the ability to write to it) private.
import { Connection } = "store-package"
// where Connection is a context.Consumer and the accompanying Provider is private
class Noun extends React.Component {
static consumes = Connection;
}Possible Issues?
Implementation: I was looking at the context changes (react/ReactContext.js and react-reconciler/ReactFiberNewContext.js) to see if using the consumer would cause any issues. From what I can tell, the consumer will always need a reference to its context, even if the consumer is not a proxy for the context, so using the consumer instead of the full context should be feasible.
Context.write: Using a consumer would cause issues if the user were allowed to use this.context to write to the context. The experimental Context.write API appears to use the context object directly, not through this.context, so using a consumer would not interfere with that. Also, if a package doesn't want users to write directly to its context, it can avoid this by only exporting its consumer.
|
That’s a good point |
|
@pshrmn Thats good feedback. One consideration is that we don’t want to necessarily keep too many concepts and object allocations around by default. It’s also a common useful pattern to allow shadowing contexts to be able to work around issues. As a library author I’m sympathetic to constraining it in certain use cases too. So if we start with the full context we still have the option to add the ability to pass a Consumer too. That way you can just think of the whole context as a thing but libraries can expose write only versions of those things. So I think we can actually get what you want without blocking this RFC as a first step. |
|
So, React Team, are you going to try to phase out the ability to send store in directly to components in the future like Redux just did? If you're thinking about it stop. Many of us test with shallow specifically by passing props directly which is how we actually TDD via isolated tests already. And it's not just a matter of "find another way", it's going to break all our tests as well as we have many benefits we gain by passing in store from tests. |
|
It's more enzyme problem:
|
|
I do not want to use provider in my tests. Period. I'm not interested in using the React Testing Library, I have no need for it when enzyme works just fine. I don't want to see react moving to a place where you are only able to write mostly integration tests but hard to write isolated tests, and possibly see shallow die. I've been using sending in a store as a prop with shallow because I have a certain way I like to TDD Redux and React. It allows for simple tests and gives me design feedback. I do not want to use context or provider, I don't need it in my lower level tests. This is exactly the problem, devs in the React community don't understand how to write isolated tests. You don't want to use mount for that and at least be able to shallow if that's how you like to test. That tells you the API is a good one if you're able to do that easily. VueJS even gets that point https://vue-test-utils.vuejs.org/guides/common-tips.html. this isn't just a matter of taste, it's a direct impact on testing and flexibility in how to do so and ability to hone in easily with higher and lower level tests without a bunch of "crap" in your way, and not having to mock a lot. With mount you have to inject a lot more, it's more verbose, it's harder to maintain, more to write, etc. The whole point of mount is for integration testing. |
|
Could you please clarify - what exactly is broken here? What does not work anymore or would be broken in a future? I also a shallow-rendering follower, but I could not see any obstacles due to this RFC. |
|
I'm very concerned with the direction Facebook has been going in regards to ignoring isolated (unit) testing and potential to killing the ability to shallow with tools like enzyme. I do not want to use Dodd's testing lib. We're already seeing Redux make changes such as outlined here. The changes that are going to cause friction (and break existing shallow tests which have been bread and butter for TDD and has worked well despite people like Dodds discounting it) for many devs are devs who are testing React components shallowly and it does not make sense to have to use something like provider in our tests. But the way React is going with requiring stuff like Context API in the future, and possibly doing the same (I hope not) which is to prevent being able to pass a store as a prop from shallow tests. If that happens with React (not able to send in store as a prop anymore like Redux did) This would not only break existing shallow tests (which Redux v6 will result in for many devs) but limits devs who want to still continue to test in isolation using shallow as the approach, small focused tests which are easy to maintain and where we do want to be able to pass in simple stores (dummies) for direct testing not through mount and the Provider API. We have ways we test where we do not need that heaviness in our tests. Vue supports shallow tests. I just don't want to see React go so far that we can't shallow anymore. It's already getting hard to shallow with promoting so many wrapping and nested components, as well as now we're starting to get totally coupled to having to use provider in our tests and mount because of so much nesting and use now of Context API. We don't all want that. So while this RFC may not impact it, other things are, the way React is requiring things going forward and not listening to devs who do not want to be tied to using mounted components in tests. I see things moving in a direction that's not supportive of isolated testing and TDD anymore as much. That is directly a result of the lack to me of devs not even realizing this because most do not write isolated tests and many have shunned shallow for bad reasons because they don't realize shallow is providing you design feedback and you have to understand how to use it properly and for the right reasons. I do not subscribe to the recent bandwagon of "Write mostly integration tests" so I hope the React team realizes the community is not all in agreement on that and that we want React to remain flexible in ways where we can test directly without the cruft using tools we have all used for a long time now like enzyme shallow. We need the ability to inject stuff into react without the cruft on the React side limiting us on that due to upcoming changes. react-router v4 also really put a bad taste on a lot of devs mouths when you wrote tests, if you used Link, you had to now couple your tests to react-router because you had to start including dependencies in tests that didn't care anything about . So I just see the entire ecosystem here becoming more messy and less simple and harder to test in isolation as this evolves. I'm seeing magic added to React more and more, promoting only integration tests, and losing simplicity and ability to test in isolation is getting harder and harder is a huge issue for devs who se value in more isolated tests over integration tests (test pyramid). At this point React is basically a framework or very close to one now IMO due to the way it's been going lately. @theKashey I've ready your article before (which BTW was great and thank you for writing what I don't have to now). We agree completely and we both test the same way. I am starting a React TDD course and when I see stuff that now limits my ability to shallow, this is a big deal to us TDD'ists or devs who want to shallow easily and get that design feedback not just "safety". |
|
Personally I am trying to solve this puzzle as a puzzle.
Today React broke your way to code and test, better to say - your taste or smell. But it's not about React, but about your own code, your way to write it (more about DI/mocking stuff than the code itself). The real problem here is only one, and it's not related to this issue, it just how it is - enzyme is not a part of React, while behavior of react-test-renderer is not acceptable by majority. And does not contain shallow renderer at all (which is not a problem for limited-depth component). |
View formatted RFC
Note: I just wrote up the RFC. The semantics were designed by @sebmarkbage.