Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deprecate context object as a consumer and add a warning message #13829

Merged
merged 14 commits into from
Oct 12, 2018
Merged

Deprecate context object as a consumer and add a warning message #13829

Changes from 1 commit
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
c902bb5
Deprecate context object as a cosumer and add a warning message
trueadm Oct 11, 2018
bd3495c
change to Object.defineProperties
trueadm Oct 11, 2018
6d7d7c0
updated comments
trueadm Oct 12, 2018
773d69d
fix typo
trueadm Oct 12, 2018
22f4359
address feedback
trueadm Oct 12, 2018
e39a444
fix flow
trueadm Oct 12, 2018
40fcddb
prettier
trueadm Oct 12, 2018
4b0c8a7
Warn against Context.Consumer.Consumer
trueadm Oct 12, 2018
f174b6e
more changes based on feedback
trueadm Oct 12, 2018
59917cd
move line
trueadm Oct 12, 2018
072b0d4
Add another warning
trueadm Oct 12, 2018
26d6d2c
address more feedback
trueadm Oct 12, 2018
d058f6e
prettier
trueadm Oct 12, 2018
9149b17
address nit
trueadm Oct 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 2 additions & 16 deletions packages/react/src/ReactContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,18 +78,12 @@ export function createContext<T>(
const consumer = {
Copy link
Collaborator

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

$$typeof: REACT_CONTEXT_TYPE,
_context: context,
_calculateChangedBits: context._calculateChangedBits,
Provider: context.Provider,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make <Context.Consumer.Provider> warn as well.

unstable_read: context.unstable_read,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's assigned right after, no? This is undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the line above, my bad.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I think of it Context.Consumer.unstable_read() should also warn.

};
// $FlowFixMe: Flow complains about not setting a value, which is intentional here
Object.defineProperties(consumer, {
Copy link
Collaborator

Choose a reason for hiding this comment

The 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?

Copy link
Collaborator

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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: {
get() {
return context._calculateChangedBits;
},
set(_calculateChangedBits) {
context._calculateChangedBits = _calculateChangedBits;
},
},
_currentValue: {
get() {
return context._currentValue;
Expand Down Expand Up @@ -119,14 +113,6 @@ export function createContext<T>(
return context.Consumer;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if you render <Context.Consumer.Consumer>? Seems like that should also warn because it also relies on objects being shared.

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.

Copy link
Contributor Author

@trueadm trueadm Oct 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

The 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 <Context /> as React's internals will always read/write from the context and never the consumer – the backwards compatibility is there for cases where libraries might try and read/write to the consumer for whatever reason. I'll address the nested Consumer.Consumer issue.

},
},
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;
Expand Down