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

contextType: convenience API for reading context in a class #65

Merged
merged 1 commit into from
Oct 23, 2018

Conversation

gaearon
Copy link
Member

@gaearon gaearon commented Oct 19, 2018

View formatted RFC

Note: I just wrote up the RFC. The semantics were designed by @sebmarkbage.

@pshrmn
Copy link

pshrmn commented Oct 19, 2018

  1. context.Consumer is essentially a proxy for Context, correct? i.e. static contextType can be a consumer. I ask because it would be nice if packages can export their context consumer, but keep the provider private.
import { Consumer } from "some-package";
class Button extends React.Component {
  static contextType = Consumer;
}
  1. prevContext was removed from componentDidUpdate in v16, with the recommendation to store the previous context value on the class instance. Will this be the same recommendation for contextType or will prevContext be brought back?
class Button extends React.Component {
  static contextType = context;
  componentDidMount() {
    this.prevContext = this.context;
  }
  componentDidUpdate() {
    if (this.context !== this.prevContext) {
      // ...
    }
  }
}

@jquense
Copy link

jquense commented Oct 19, 2018

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 withFoo(withBar(Component)) for relay, react-router, react-intl etc.

@AlexGalays
Copy link

AlexGalays commented Oct 19, 2018

I'm in favor of not doing anything.

  • Aren't classes slowly going to get phased out?

  • subscribing to only one context is actually easy with the <Consumer/> component. Subscribing to multiple contexts is what's annoying in vanilla react but this spec doesn't change that.

  • We can easily write a connectToContexts([Context1, Context2], (value1, value2) => ({ value1, value2 }) kind of helper today.

  • This seems difficult to type without React specific hacks in flow/TS.

@gaearon
Copy link
Member Author

gaearon commented Oct 19, 2018

context.Consumer is essentially a proxy for Context, correct? i.e. static contextType can be a consumer. I ask because it would be nice if packages can export their context consumer, but keep the provider private.

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.

prevContext was removed from componentDidUpdate in v16, with the recommendation to store the previous context value on the class instance. Will this be the same recommendation for contextType or will prevContext be brought back?

This componentDidUpdate argument is already taken by the snapshot so I don’t think we’ll bring it back. Storing it manually still works though.

our apps are more and more looking like withFoo(withBar(Component)) for relay, react-router, react-intl etc.

We have something in the works that helps with this. Will likely post an RFC next week.

Aren't classes slowly going to get phased out?

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.

This seems difficult to type without React specific hacks in flow/TS.

I don’t see why, can you explain more?

@jfo84
Copy link

jfo84 commented Oct 19, 2018

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So excited for this proposal! Can you explain why this limit is here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Static typing and performance reasons.

@AlexGalays
Copy link

AlexGalays commented Oct 19, 2018

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?

This change is just a shortcut for <Consumer />; you still need a matching <Producer/> somewhere above.

@strayiker
Copy link

strayiker commented Oct 19, 2018

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
    ...
  }
}

magic

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;
    ...
  }
}

@jfo84
Copy link

jfo84 commented Oct 19, 2018

@AlexGalays Okay got it. Still a big fan of the change

@mAAdhaTTah
Copy link
Contributor

@strayiker The API for multiple contexts would be much worse if you did it w/ inheritance.

@strayiker
Copy link

@mAAdhaTTah Totally agree. But I assume that this RFC for simplest case with single context.

@cangoektas
Copy link

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;
  }
}

@TrySound
Copy link
Contributor

@cangoektas contextTypes is reserved by the old context api

@pshrmn
Copy link

pshrmn commented Oct 19, 2018

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.

I'm probably missing some context 😏 that you'll cover in your explanation. My understanding of the context/context.Consumer parity issue was (based on facebook/react#13829) that the context could be used as an element and that properties could be chained off of the consumer a la context.Consumer.Provider.

As far as actual design goes, this.context would be read only (like the consumer), right? Again, I'm probably missing some context, but I personally would prefer for static contextType to be a consumer, not the full context.

@gaearon
Copy link
Member Author

gaearon commented Oct 19, 2018

I'd also love to see support for multiple contexts if we can agree on an intuitive API.

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.
Copy link
Member

@aweary aweary Oct 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are the specific problems with the new context that have slowed adoption, and that this solves? Accessing context in lifecycles?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

¯\(ツ)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We warn immediately if you pass something else.

@theKashey
Copy link

observedBits is no longer a thing?

@gaearon
Copy link
Member Author

gaearon commented Oct 20, 2018

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 Consumer.

Copy link

@pshrmn pshrmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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.

@sebmarkbage
Copy link
Collaborator

That’s a good point

@sebmarkbage
Copy link
Collaborator

@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.

@gaearon gaearon restored the gaearon-patch-3 branch October 24, 2018 05:37
@bvaughn bvaughn deleted the gaearon-patch-3 branch December 20, 2018 21:38
@dschinkel
Copy link

dschinkel commented Dec 21, 2018

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.

@theKashey
Copy link

It's more enzyme problem:

  • you can dive into Provider. Shallow rendering gives you rich API
  • you will be able to Context.write, to update default value before the render. One day.
  • you can use dependency mocking to create contexts with different default values. Just a matter of taste.

@dschinkel
Copy link

dschinkel commented Dec 21, 2018

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.

@theKashey
Copy link

Could you please clarify - what exactly is broken here? What does not work anymore or would be broken in a future?
Could you provide some code you are(were) happy about, and not happy after this change?

I also a shallow-rendering follower, but I could not see any obstacles due to this RFC.

@dschinkel
Copy link

dschinkel commented Dec 21, 2018

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".

@theKashey
Copy link

Personally I am trying to solve this puzzle as a puzzle.

  • limited depth components, you may confidently test with mount
  • and extend providing values for slots in the integration component, you may test with shallow

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.