-
Notifications
You must be signed in to change notification settings - Fork 559
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
New method to extract context value #47
Conversation
Except the problem you described here (which is what new context api was created for) you are missing another big problem with your proposal: context is not global variable, you should be able to inject its value with provider for different subtrees. |
|
||
# Unresolved questions | ||
|
||
- How React should propagate context value changes to consumers that are using |
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.
This is the main question. From what I understand context is useful because:
- It re-renders the subtree when its value changes.
- It preserves parent-child relationship.
The proposed API does not provide any of those features. In this case it can very simply be implemented in user-space:
let lang, theme;
const getLanguage() => lang;
const getTheme() => theme;
function ThemeAndLanguageConsumer({ children }) {
let language = getLanguage();
let theme = getTheme();
return children({ language, theme });
}
@TrySound, this part remains the same with the new method introduced. You access |
We have some future API ideas that address this problem (in addition to others). We're not ready to share them yet but just leaving a note that we've looked into this. (Edit: those future ideas are not related to HOC pattern Andrew suggested) |
You can have a user-space solution like this: function ThemeAndLanguageConsumer({ children }) {
return
userspaceSolution(LanguageConsumer, ThemeConsumer, (language, theme) =>
children({ language, theme }));
} |
I also made one. example | code <Provide
theme={[themeContext, this.state.theme]}
color={[colorContext, this.state.color]}
>
<Consume
theme={themeContext}
color={colorContext}
>
{
({color, theme}) => <Box color={color} theme={theme}/>
}
</Consume>
</Provide> Really simple export const Provide = ({children, ...providers}) => {
const pKeys = Object.keys(providers)
return pKeys.reduceRight((nested, key) => {
const [{Provider}, value] = providers[key]
return <Provider value={value}>{nested}</Provider>
},
children)
}
export const Consume = ({children, ...consumers}) => {
const pKeys = Object.keys(consumers)
let lastKey, values = {} // collect values from consumers
return pKeys.reduceRight((nested, key) => {
const Consumer = consumers[key]
return prop => {
if (lastKey) values = {...values, [lastKey]: prop}
lastKey = key
return <Consumer>{nested}</Consumer>
}
}, prop => children({...values, [lastKey]:prop}))
() // after every consumer is wrapped, unwrap (call) one dummy level
} |
@TrySound, current proposal does not affect the way how context value is propagated. The value does not become global and only propagated by Provider component, in the way it works right now. @streamich, the mentioned useful properties are preserved with the suggested method. I feel really bad I haven't made this clear in the RFC text. Thank you for the feedback, I'm looking for updating some parts to make the design section easier to read. |
@gaearon, thank you for answering. I'm glad the React team is also looking at solving mentioned problems. I understand the motivation behind doing deeper research before making public changes. Do you think this RFC should be kept open and evolving (I'm even more interested in teaching section) or it may be easier for all of us to just wait for the team to publish their solution? |
I think it’s fine to keep open if you find the discussion valuable. Our solution is similar in spirit but it’s significantly broader in scope so it will take more time to bake. |
BTW: Is this a hidden feature or it should not be used like this? const MyComponent = () => JSON.stringify(themeContext._currentValue) (I know I don't get re-renders.) |
@ackvf Well, if it does not have |
@streamich That's what I thought. Maybe I should have asked right away - why it is not documented or even suggested here to solve @alexeyraspopov problem? function ThemeAndLanguageConsumer({ children }) {
let language = LanguageContext._currentValue;
let theme = ThemeContext._currentValue;
return children({ language, theme });
} |
Guys, with all your suggestions what would you like to see here rendered? function Language() {
return LanguageContext._currentValue;
}
<>
<LanguageContext.Provider value='en'>
<Language />
</LanguageContext.Provider>
<LanguageContext.Provider value='fr'>
<Language />
</LanguageContext.Provider>
</> |
@TrySound Ignore the implications. Of course it breaks about everything, but if a user knows what he's doing, why it is not documented? Like class MyClass extends React.Component {
componentDidMount() {
setTimeout(this.forceUpdate, 100);
}
render() {
const val1 = SomeContextA._currentValue
const val2 = SomeContextB._currentValue
const val3 = SomeContextC._currentValue
const val4 = SomeContextD._currentValue
return (...)
}
} BTW, your example should correctly return |
@ackvf It's obviously not public api. And I'm not sure anybody will know what he is doing here. I'm asking seriously what is the result in my example? It's important case which shouldn't be broken. |
@TrySound I think you are likely to be correct, but for some edge use cases it would be beneficial to be aware of, like in the suggested RFC or for library authors. Or is the preferred approach to reverse-engineer the implementation rather than to read docs in order to build something? Anyway, I don't get your point with the example. Are you suggesting that returning |
@ackvf You are trying get the value from global variable which has only ONE value. The idea of context is isolation this value in subtree. So many values can be kept in parallel. Why react should introduce potentially unsafe way to use things. You talking about reusable library, but it's even bigger problem for them. Libraries shouldn't not contain global variables. Every instantiation will conflict with another. I just don't understand why it's not obvious. |
The docs say
Since there is nothing regarding
And guess what, it really indeed returns Moreover, given the old implementation of context api, I would actually not expect to access a global variable like you suggest, but instead to access a value "somehow magically" available in a given tree. PS: If |
This RFC does not enforce to read from some global value. Also, no one is going to rely on |
Than I guess we should request documentation for |
I don't think it is a solution. The value may not be available at the moment the render function is called and it doesn't allow to make a subscription for subsequent value updates. Also, |
Just today I've tried to get value from context in |
@theKashey You could use the property |
@theKashey You can safely use this way until facebook/react#13139 will be landed class Component extends React.Component {
value = null
componentDidMount() {
this.value.method()
}
render() {
return <Context.Consumer>
{value => {
this.value = value;
return (
<div></div>
)
}}
<Context.Consumer>
}
} |
I am ok with component splitting. It's much easier to test them. |
I don't recommend anyone to use We are experimenting with an API like the one suggested in this RFC. Note it won't work in commit phase lifecycles — only in the render phase. The distinction is confusing enough at the moment that we're going to keep the API unstable until we solve some other issues. |
I think this PR can now be just closed, given the fact that hooks are covering the usage of context in the way that was described here. I'm so excited about the opportunity to try it. Thanks React team for all their effort! ❤️ |
👍 Thanks for writing up the proposal! |
View formatted RFC