-
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 10 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 |
---|---|---|
|
@@ -13,6 +13,7 @@ import type {ReactContext} from 'shared/ReactTypes'; | |
|
||
import invariant from 'shared/invariant'; | ||
import warningWithoutStack from 'shared/warningWithoutStack'; | ||
import warning from 'shared/warning'; | ||
|
||
import ReactCurrentOwner from './ReactCurrentOwner'; | ||
|
||
|
@@ -67,9 +68,60 @@ export function createContext<T>( | |
$$typeof: REACT_PROVIDER_TYPE, | ||
_context: context, | ||
}; | ||
context.Consumer = context; | ||
|
||
context.unstable_read = readContext.bind(null, context); | ||
|
||
let hasWarnedAboutUsingNestedContextConsumers = false; | ||
|
||
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 = { | ||
$$typeof: REACT_CONTEXT_TYPE, | ||
_context: context, | ||
_calculateChangedBits: context._calculateChangedBits, | ||
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 |
||
unstable_read: context.unstable_read, | ||
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. It's assigned right after, no? This is 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. Moved the line above, my bad. 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. Now that I think of it |
||
}; | ||
// $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. |
||
_currentValue: { | ||
get() { | ||
return context._currentValue; | ||
}, | ||
set(_currentValue) { | ||
context._currentValue = _currentValue; | ||
}, | ||
}, | ||
_currentValue2: { | ||
get() { | ||
return context._currentValue2; | ||
}, | ||
set(_currentValue2) { | ||
context._currentValue2 = _currentValue2; | ||
}, | ||
}, | ||
Consumer: { | ||
get() { | ||
if (!hasWarnedAboutUsingNestedContextConsumers) { | ||
hasWarnedAboutUsingNestedContextConsumers = true; | ||
warning( | ||
false, | ||
'Rendering <Context.Consumer.Consumer> is not supported and will be removed in ' + | ||
'a future major release. Did you mean to render <Context.Consumer> instead?', | ||
); | ||
} | ||
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 |
||
}, | ||
}, | ||
}); | ||
// $FlowFixMe: Flow complains about missing properties because it doesn't understand defineProperty | ||
context.Consumer = consumer; | ||
} else { | ||
context.Consumer = context; | ||
} | ||
|
||
if (__DEV__) { | ||
context._currentRenderer = null; | ||
context._currentRenderer2 = null; | ||
|
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.
Nit: I'd probably call this
Consumer