-
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
Deprecate context object as a consumer and add a warning message #13829
Changes from 3 commits
c902bb5
bd3495c
6d7d7c0
773d69d
22f4359
e39a444
40fcddb
4b0c8a7
f174b6e
59917cd
072b0d4
26d6d2c
d058f6e
9149b17
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 |
---|---|---|
|
@@ -8,6 +8,7 @@ | |
*/ | ||
|
||
import type {ReactProviderType, ReactContext} from 'shared/ReactTypes'; | ||
import lowPriorityWarning from 'shared/lowPriorityWarning'; | ||
import type {Fiber} from 'react-reconciler/src/ReactFiber'; | ||
import type {FiberRoot} from './ReactFiberRoot'; | ||
import type {ExpirationTime} from './ReactFiberExpirationTime'; | ||
|
@@ -1096,12 +1097,36 @@ function updateContextProvider( | |
return workInProgress.child; | ||
} | ||
|
||
let hasWarnedAboutUsingContextAsConsumer = false; | ||
|
||
function updateContextConsumer( | ||
current: Fiber | null, | ||
workInProgress: Fiber, | ||
renderExpirationTime: ExpirationTime, | ||
) { | ||
const context: ReactContext<any> = workInProgress.type; | ||
let context: ReactContext<any> = workInProgress.type; | ||
// The logic below for Context differs depending on PROD or DEV mode. In | ||
// DEV mode, we create a separate object for Context.Consumer that acts | ||
// like a proxy to Context. This proxy object adds unecessary code in PROD | ||
// so we use the old behaviour (Context.Consumer references Context) to | ||
// reduce size and overhead. The separate object references context via | ||
// a property called "_context", which also gives us the ability to check | ||
// in DEV mode if this property exists or not and warn if it does not. | ||
if (__DEV__) { | ||
if (workInProgress.type._context === undefined) { | ||
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. Can this just be |
||
if (!hasWarnedAboutUsingContextAsConsumer) { | ||
hasWarnedAboutUsingContextAsConsumer = true; | ||
lowPriorityWarning( | ||
false, | ||
'You are using the Context from React.createContext() as a consumer.' + | ||
'The correct way is to use Context.Consumer as the consumer instead. ' + | ||
'This usage is deprecated and will be removed in a future major release.', | ||
); | ||
} | ||
} else { | ||
context = workInProgress.type._context; | ||
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. Can you explain why we read it differently in DEV and PROD? It would help to have a written explanation of DEV/PROD matrix for Consumer/Context object and how behavior changes for each. 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. Can you reference me to another matrix? I updated the comment 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. There's none. I just meant if you could write up of what's supposed to change.
That would be easier for me to review since I can focus on intent separately from code. 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've put a big nice comment in there that explains things better now. I'll aadd the matrix to this PR. 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. This can also be just |
||
} | ||
} | ||
const newProps = workInProgress.pendingProps; | ||
const render = newProps.children; | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1190,12 +1190,12 @@ describe('ReactNewContext', () => { | |
|
||
function FooAndBar() { | ||
return ( | ||
<FooContext> | ||
<FooContext.Consumer> | ||
{foo => { | ||
const bar = BarContext.unstable_read(); | ||
return <Text text={`Foo: ${foo}, Bar: ${bar}`} />; | ||
}} | ||
</FooContext> | ||
</FooContext.Consumer> | ||
); | ||
} | ||
|
||
|
@@ -1558,4 +1558,31 @@ Context fuzz tester error! Copy and paste the following line into the test suite | |
} | ||
}); | ||
}); | ||
|
||
it('should warn with an error message when using context as a consumer in DEV', () => { | ||
const BarContext = React.createContext({value: 'bar-initial'}); | ||
const BarConsumer = BarContext; | ||
|
||
function Component() { | ||
return ( | ||
<React.Fragment> | ||
<BarContext.Provider value={{value: 'bar-updated'}}> | ||
<BarConsumer> | ||
{({value}) => <div actual={value} expected="bar-updated" />} | ||
</BarConsumer> | ||
</BarContext.Provider> | ||
</React.Fragment> | ||
); | ||
} | ||
|
||
expect(() => { | ||
ReactNoop.render(<Component />); | ||
ReactNoop.flush(); | ||
}).toLowPriorityWarnDev( | ||
'You are using the Context from React.createContext() as a consumer.' + | ||
'The correct way is to use Context.Consumer as the consumer instead. ' + | ||
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. This message might confuse people a little bit. Maybe make it more visual?
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. This message is still not clear @trueadm . I'm already calling Context.Consumer as a consumer. But I'm still getting the error message. import React from "react"
const defaultState = {
dark: false,
toggleDark: () => {},
}
const ThemeContext = Context.Consumer(defaultState)
// Getting dark mode information from OS!
// You need macOS Mojave + Safari Technology Preview Release 68 to test this currently.
const supportsDarkMode = () =>
window.matchMedia("(prefers-color-scheme: dark)").matches === true
class ThemeProvider extends React.Component {
state = {
dark: false,
}
toggleDark = () => {
let dark = !this.state.dark
localStorage.setItem("dark", JSON.stringify(dark))
this.setState({ dark })
}
componentDidMount() {
// Getting dark mode value from localStorage!
const lsDark = JSON.parse(localStorage.getItem("dark"))
if (lsDark) {
this.setState({ dark: lsDark })
} else if (supportsDarkMode()) {
this.setState({ dark: true })
}
}
render() {
const { children } = this.props
const { dark } = this.state
return (
<ThemeContext.Provider
value={{
dark,
toggleDark: this.toggleDark,
}}
>
{children}
</ThemeContext.Provider>
)
}
}
export default ThemeContext
export { ThemeProvider } 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. @laurosilvacom you should be using ThemeContext = createContext(...) and then, doing ThemeContext.Consumer and ThemeContext.Provider 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'm not calling 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 was suggesting you should call it and use it that way. I can help more when I’m back in the office in a few days. 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. Sounds good. I appreciate the help. 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 don't understand what you're trying to do. You should create context like this: const ThemeContext = React.createContext(defaultState) // NOT "Context.Consumer(defaultState)" and then subscribe to it either like this: class Button extends Component {
static contextType = ThemeContext; // NOT ThemeContext.Consumer
render() {
// ...
}
} or like this: class Button extends Component {
render() {
return (
// NOT <ThemeContext>
<ThemeContext.Consumer>
{theme => ...}
</ThemeContext.Consumer>
);
}
} Does that help? 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. Yes! I did not realize I needed to subscribe to it by calling I had: <ThemeContext>
{theme => ...}
</ThemeContext> Thanks @gaearon! 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. Right, which is why the warning says:
Do you still feel it was unclear? Which part could be improved? Thanks! 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. An error message that's direct would help.
|
||
'This usage is deprecated and will be removed in a future major release.', | ||
{withoutStack: true}, | ||
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. Why no stack? This is exactly the kind of warning where I think the stack would be highly valuable. 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. n00b question: how does one generate a stack with |
||
); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,7 +67,61 @@ export function createContext<T>( | |
$$typeof: REACT_PROVIDER_TYPE, | ||
_context: context, | ||
}; | ||
context.Consumer = context; | ||
|
||
if (__DEV__) { | ||
// A separate object, but proxies back to the original context object for | ||
// backwards compatibility. It has a different $$typeof, so we can properly | ||
// warn for the incorrect usage of Context as a Consumer. | ||
const consumer = { | ||
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. Nit: I'd probably call this |
||
$$typeof: REACT_CONTEXT_TYPE, | ||
_context: context, | ||
Provider: context.Provider, | ||
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. Let's make |
||
}; | ||
// $FlowFixMe: Flow complains about not setting a value, which is intentional here | ||
Object.defineProperties(consumer, { | ||
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. Is it better to have consumer proxy to context, or the other way around? Why? 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. Whatever we read/write from/to most. I’m actually not sure which one that is. 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 checked and the context gets written to the most in our tests. |
||
_calculateChangedBits: { | ||
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.
|
||
get() { | ||
return context._calculateChangedBits; | ||
}, | ||
set(_calculateChangedBits) { | ||
context._calculateChangedBits = _calculateChangedBits; | ||
}, | ||
}, | ||
_currentValue: { | ||
get() { | ||
return context._currentValue; | ||
}, | ||
set(_currentValue) { | ||
context._currentValue = _currentValue; | ||
}, | ||
}, | ||
_currentValue2: { | ||
get() { | ||
return context._currentValue2; | ||
}, | ||
set(_currentValue2) { | ||
context._currentValue2 = _currentValue2; | ||
}, | ||
}, | ||
Consumer: { | ||
get() { | ||
return context.Consumer; | ||
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. What if you render Which makes me think: should the warning move into these getters instead? Fire for first getter accessed, ignore the rest. This could also nicely let us warn once per context type. 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. Moving the warning into the getters will stop the error from coming up for just using |
||
}, | ||
}, | ||
unstable_read: { | ||
get() { | ||
return context.unstable_read; | ||
}, | ||
set(unstable_read) { | ||
context.unstable_read = unstable_read; | ||
}, | ||
}, | ||
}); | ||
// $FlowFixMe: Flow complains about missing properties because it doesn't understand defineProperty | ||
context.Consumer = consumer; | ||
} else { | ||
context.Consumer = context; | ||
} | ||
context.unstable_read = readContext.bind(null, context); | ||
|
||
if (__DEV__) { | ||
|
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.
unnecessary