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

Document class contextType as the primary consuming mechanism #1265

Closed
wants to merge 1 commit into from
Closed

Document class contextType as the primary consuming mechanism #1265

wants to merge 1 commit into from

Conversation

sebmarkbage
Copy link
Contributor

facebook/react#13728

This documents the Class.contextType API.

The intention here is to document this as the primary way to consume a Context from a class, and since classes are still the mainstream component API, it's the primary way to consume a Context period.

The Consumer API is documented as the way to consume a Context from a function component.

As a result I could remove the following examples:

Accessing Context in Lifecycle Methods - Accessing context in life-cycles is now easy in since the primary way makes that the default. Technically this is still relevant for multiple context in a single class, but it's such an edge case that I feel like it belongs on Stack Overflow or some recipes section. Not the primary documentation.

Forwarding Refs to Context Consumers - While this is still something you might want to do, this is not different than forwarding refs in general. It used to be that this pattern was unique in that when you converted from a class to use Consumer + forwardRef, you would get into this awkward situation. However, if the primary API is to use .contextType on a class, then this situation doesn't happen so doesn't seem to add the confusion.

Consuming Context with a HOC - This pattern is possibly still relevant but it's only relevant because the render prop syntax is kind of a mess when you compose many of them. However, it looks like we're not going to be recommending this pattern and instead look to make it easier to consume context in other ways like the unstable_read proposal. Since this pattern isn't critical even before that's stable I think we should just remove it.

@reactjs-bot
Copy link

Deploy preview for reactjs ready!

Built with commit c9fe3768478d1533970ed73d1b62ce7438a99403

https://deploy-preview-1265--reactjs.netlify.com

@reactjs-bot
Copy link

reactjs-bot commented Oct 16, 2018

Deploy preview for reactjs ready!

Built with commit d7b724e

https://deploy-preview-1265--reactjs.netlify.com

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

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

I think this looks good overall. Left some thoughts.


### `Consumer`
All consumers that are descendants of a Provider will re-render whenever the Provider's `value` prop changes. The propagation from Provider to its descendant consumers is not subject to the `shouldComponentUpdate` method, so the consumer is updated even when an ancestor component bails out of the update.
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should clarify that context is subject to a component's own shouldComponentUpdate method, just not its ancestors. (Seems like this wording could be misleading.)

render() {
let value = this.context;
/* render soemthing based on the value of MyContext */
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth showing shouldComponentUpdate here so we can illustrate this.context vs the nextContext param? Or would that muddy the waters too much?

If not here, we should mention that param/behavior somewhere now that the "Accessing Context in Lifecycle Methods" section is gone.

}
render() {
let value = this.context;
/* render soemthing based on the value of MyContext */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/* render soemthing based on the value of MyContext */
/* render something based on the value of MyContext */

@alexkrolick
Copy link
Collaborator

I think the HOC examples should be kept around. I'm not sure that consuming just a single context from a component is actually that common. The new API works well as a 1-1 replacement for some usages of legacy context, but most apps I've seen use a combination of HOCs and render props to inject data providers and there are usually more than one (for example react-intl injectIntl + Redux connect, plus some app-specific login context, navigation context, etc.).

@brunolemos
Copy link

brunolemos commented Oct 18, 2018

This looks great overall!

Technically this is still relevant for multiple context in a single class, but it's such an edge case that I feel like it belongs on Stack Overflow

I'm not sure this case is so edge, it was the first question that came to my mind when I knew about this

or some recipes section

Is it supported to access multiple contexts from this.context? If so, yes please add info about this somewhere from the beginning.

I imagine something like this:

static contextTypes = {
    myContext1: MyContext1,
    myContext2: MyContext2,
}

or

static contextTypes = [MyContext1, MyContext2]

EDIT: Just noticed this was already requested here: facebook/react#13728 (comment)

@gaearon
Copy link
Member

gaearon commented Oct 19, 2018

You can still use render prop API for cases when you need that. This just attempts to cover the 80% case.

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.

None yet

8 participants