-
Notifications
You must be signed in to change notification settings - Fork 47k
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
Support class component static contextType attribute #13728
Conversation
const context = isContextConsumer | ||
? getMaskedContext(workInProgress, unmaskedContext) | ||
: emptyContextObject; | ||
let isContextConsumer = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we name this isLegacyContextConsumer
to be more specific?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add a test that confirms that the component that receives a context change will still re-render even if sCU returns false (also PureComponent)? I know you said the test passes but we should have it anyway to prevent a regression.
Also this PR makes it even more clear that we should implement legacy context on top new context.
ReactDOM: size: 🔺+0.5%, gzip: 🔺+0.3% Details of bundled changes.Comparing: 13965b4...353cc7e react-dom
react-art
react-test-renderer
react-reconciler
react-native-renderer
scheduler
Generated by 🚫 dangerJS |
Should we warn if you put |
The existing test kind of obviates the need for this by confirming that sCU isn't even called. (I don't think there's a way to actually call sCU with the current implementation.)
Sure! |
1. Renamed isContextConsumer to isLegacyContextConsumer. 2. Added DEV warning for unsupported contextType on stateless functional component.
Yeah but the point is to prevent the implementation from regressing :D |
That's fair! #13729 |
if ( | ||
typeof contextType === 'object' && | ||
contextType !== null && | ||
typeof contextType.unstable_read === 'function' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this extra check? Seems like we can just throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't, since we also provide a nice DEV mode warning if it's not a function. I'll remove this type check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, if we don't check in at least the instantiation path, we'll throw before our DEV warning. So I think it's best to leave these checks in place after all.
If you feel strongly about it, let's talk and come up with another plan. I think the DEV warnings are useful to preserve though.
Disregard. I'll just move the warning earlier. #13736
Was this API proposed only for migration purposes? I've been using new Context for awhile now, and it works great in most cases, but starts to feel boilerplately when I need to access context from inside a class method. The above API seems to capture that use case perfectly. I'd love to start using it. |
Sure you can start using it. Yeah it’s mostly for easier migration but you can use it in either way if you like. |
Was it considered to support consuming multiple context providers with the API? static contextTypes = {
foo: FooContext,
bar: BarContext,
};
this.context.foo
this.context.bar |
Yes, it was considered and didn't seem worth it. Consuming multiple contexts is an edge case / advance pattern that we didn't want to add overhead to support with this new API. |
* Support class component static contextType attribute
Hopefully multiple contexts will be reconsidered in the future. I'm not sure if I've been misusing them but in larger apps I tend to have a |
Yes please consider supporting multiple contexts. It's not so edge. I just made the exact same suggestion here before reading the comments above: reactjs/react.dev#1265 (comment) |
I, too, would love to see multiple contexts supported. That said, I think a decorator / HoC might make this palatable in userland, relatively simply. I think something like this might work const addContext = (name, context) => Orig => class extends Component {
static contextType = context;
render(){
return <Orig {...this.props} name={this.context} />
}
} And then @addContext("userContext", userContext)
@addContext("uiContext", uiContext)
class SomeComponent extends Component {
componentDidUpdate(prevProps, prevState){
let { userContext, uiContext } = this.props;
//...
}
render(){
return (
// ...
)
}
} That's of course based on the legacy decorator spec - it'd need to change for the new spec, obviously. |
I'm curious why you'd use a class component for the decorator rather than the render prop API @arackaf? |
@gaearon |
@bvaughn How to let the component be re-rendered in case |
The component will automatically re-render if its context value changes. 👍 |
* Support class component static contextType attribute
Note: the discussion for this proposal is in reactjs/rfcs#65.
The main motivation is that continuing to support legacy context makes React slower and larger, and we'd like to be able to help everyone migrate to the new context API as soon as possible. Since the new context API is currently too low-level and/or verbose when used in classes, we are considering adding this higher-level API that feels more familiar and will simplify the migration.
New static
contextType
property is supported for class components. This property can be set to the value returned byReact.createContext()
in order to consume the context value viathis.context
, e.g.:Resolves #13336
DEV warnings have been added for the following cases:
contextType
instance property.contextType
andcontextTypes
static properties.contextType
static property (e.g. mistakencontextType = Context.Provider
).contextType
property.New tests have been added to verify the following:
nextContext
params.this.context
values.shouldComponentUpdate
being called) when context provider updates.