-
Notifications
You must be signed in to change notification settings - Fork 30.6k
Add new RenderProps-style Context from React 16.3 #24509
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -75,6 +75,9 @@ const StatelessComponentWithoutProps: React.SFC = (props) => { | |
| }; | ||
| <StatelessComponentWithoutProps />; | ||
|
|
||
| // React.createContext | ||
| const ContextWithRenderProps = React.createContext('defaultValue'); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this is the way too simple test to cover the changes. I would propose following, but I am not entirely sure how to run these tests, so there might some mistake (or several) interface ContextTest = {
assert: boolean
}
const ContextWithValue = React.createContext<ContextTest>({ assert: true })
const ContextWithoutValue = React.createContext<ContextTest>()
<ContextWithValue.Context>{value => value.assert}</ContextWithValue.Context>
<ContextWithoutValue.Provider value={{ assert: true }}>
<ContextWithoutValue.Context>{value => value.assert}</ContextWithoutValue.Context>
</ContextWithoutValue.Provider>
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FredyC I'm not sure myself. The test files here don't seem to be assertion-style tests so much as a sandbox to prove that the typings work without syntax errors. I agree that there could be more code here exercising both context scenarios (with and without defaults), but I don't know that they should be executable as none of the rest of the code in this file seems to be.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well yes, tests code is not executed, because you're testing types, not React's behavior. What you have to do for failing tests is add
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @scally Are you willing to try that? Perhaps it's overkill for such relatively simple thing. It's your call.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FredyC Yes, I'll try it out and see where I get with it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am trying something like this: const RenderPropsContext = React.createContext('defaultValue');
interface RenderPropsContextTest {
assert: boolean;
}
const ContextWithDefaultValue = React.createContext<RenderPropsContextTest>({ assert: true });
const ContextWithoutDefaultValue = React.createContext<RenderPropsContextTest>();
const ContextRenderPropsConsumerComponent = (
<ContextWithDefaultValue.Consumer>
{value => value.assert}
</ContextWithDefaultValue.Consumer>
);
const ContextRenderPropsProviderConsumerComponent = (
<ContextWithoutDefaultValue.Provider value={{ assert: true }}>
<ContextWithoutDefaultValue.Consumer>
{value => value.assert}
</ContextWithoutDefaultValue.Consumer>
</ContextWithoutDefaultValue.Provider>
); // $ExpectError
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FredyC Are these tests sufficient for what you feel should be achieved here?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Strange that the error is in the full statement instead of in the argument to An additional non-erroring test where the argument is There's one last confirmation I'd like to take; if the Context is created with a default value, will the Consumer receive that value even if it's not put inside its Provider in the tree? 🤔 looks at source EDIT: The context's context has a copy of the default argument as its value, and it's only overwritten by the reconciler if the Provider is present. It's also restored to its original value once the Provider tree has been processed. So indeed it can only be |
||
|
|
||
| // Fragments | ||
| <div> | ||
| <React.Fragment> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
It should be possible to call
createContext()to create one withdefaultValueofundefined.function createContext<T>(): Context<T|undefined>override? This is one of those that look like the antipattern warned against in the docs, but I think it's correct to use here, similar to a collection constructor.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.
Hmm. I'm confused here. Calling
createContext()(i.e., no arguments) would require thedefaultValueto be optional. Otherwise you'd need to do an explicitcreateContext(undefined). Also, sinceTcan be anything, I don't understandT|undefined.FYI, I address this case of optional default values in my article on the new Context API (using the typings as they stand here, but wrapping them in one additional layer). Also note that, in my opinion (see article), it isn't just about making the default value optional. But again, I used these typings without issue.
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.
16.3 released today and the docs seem to indicate @Kovensky is correct here — they use ‘createContext’ without args so it must be a correct usage. I’ll take a look at updating later today.
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.
https://reactjs.org/docs/context.html
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.
OK, rebased this post merge conflict.
Spent some time with this tonight and it looks like the behavior of the final API is that
createContext()gives you a context object where_defaultValueis undefined.createContext('foo')gives you a context object where_defaultValueisfooGiven that, I'm a bit confused as to the correct typing for
createContext()with no params. I tried the overload @Kovensky mentioned but doing so types the resulting RenderProps function args with{} or undefined, which I don't think is correct.I suspect the overload closest to the API's usage would be
Which completely gives up type safety for RenderProps function but at least allows it to be used.
{} or undefineddoesn't allow you to still give, say, an anonymous object in the Provider'svaluefield and still be able to retrieve it later in the RenderProps.If anyone has any opinions on this they'd be welcome.
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.
Yup, I'm just saying that we shouldn't want to correct that behavior, because you can always use
Consumernot paired with aProviderso the value ofTshould not change even if you later pass in the value as a prop toProvider.I.e. if you create the context with the default value, you are always guaranteed to have a defined value wherever you use
Consumer(even not as a descendant ofProvider), but once you create the context without a default value, you are not guaranteed that defined value unless you wrap theConsumerinside aProvider, which wouldn't be able to be statically analyzed before runtime.I'm agreeing with you that it's unfortunate that we'll always have to check for undefined, but I think that should be the way the interface is structured due to the lack of compile-time guarantees of the consumer value
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.
Well, in your first example you had the comment there
/* value here will be defined, even though it's typed as optional */and that was what's confused me the most. As you have said now, you will have to check forundefinedeither way. I understand now.I see a bit of problem arising here. Developers are generally lazy and we might end up with pattern like this.
That might be a good source of runtime errors (and blaming TypeScript/React for it) in case someone really forgets to add the Provider. It's true that if anyone is using
!it's for own risk, but there are people that might just copy&paste and then be surprised.What about this approach? Would this work the same way? I am not sure if it's a correct syntax, but you get the gist. This would at least allow to override this
undefinedtype in case someone knows what they are doing.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.
Hmm.. it might not be immediately clear to the user why the second generic type param may be used, and they'll have to dig into the type defs to actually find out how they resolve.
With my suggestion (the two overloads), creating context with a default param will assert the value in the consumer to be always be defined:
in this case, all you need as a type consumer to override the
undefinedbehavior is to just provide a default value, the type inference will work for you or you can explicitly set the generic type parameter, and as long as the default value is passed in as a parameter, the consumer render props will always be definedThere 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.
@ferdaber I'm trying to use the TypeScript 2.8 features you mentioned in this definition, but it looks like that's not possible yet?
I've updated the headers to
// TypeScript Version: 2.8but that results in a linting error.According to this comment, we can only use TypeScript up to 2.7 right now, so I may not be able to use the definition you specify.
#24586 (comment)
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.
ah, that's too bad, we'll keep it in the backburner then