-
-
Notifications
You must be signed in to change notification settings - Fork 2
Context updates #5
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
Context updates #5
Conversation
content/docs/context.md
Outdated
@@ -91,8 +91,12 @@ A more complex example with dynamic values for the theme: | |||
|
|||
## Consuming Multiple Contexts | |||
|
|||
To keep context re-rendering fast, React needs to make each context consumer a separate node in the tree. |
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.
Why mention this? It's an implementation detail.
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.
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.
cc: @gaearon I feel like this is a rationalization, because we would likely use this same API even if it didn't happen to be good for performance. It's the way it is for multiple reasons, not only that one.
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.
Feel free to suggest an alternative :-) It's the only way I've been able to get people to put behind their preconception that the extra nesting is "ugly". I feel that if we don't double down on this and somehow motivate it (I don't care much how exactly), we'll keep getting the same confused looks.
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.
I suppose. Though the people who think the extra nesting is ugly aren't wrong :D
To me, the main motivation for using special component types is that it decouples context from classes. So when we introduce a new component API in the future, context will already just work. This is one of the rationales behind higher-order component pattern we've been discussing.
Maybe we should just ship useContext
. Or provide a recipe for it, since it's only a few lines.
But if you feel this sentence is a good way to head off objections, I'm cool with leaving it in, even if I don't agree it's the main reason we designed it this way.
content/docs/context.md
Outdated
@@ -109,5 +113,5 @@ A more complex example with dynamic values for the theme: | |||
|
|||
> Note | |||
> | |||
> React previously shipped with an experimental context API. The old API will be supported in all 16.x releases, but applications using it should migrate to the new version. The legacy API will be removed in React 17. Read the [legacy context docs here](/docs/legacy-context.html). | |||
> React previously shipped with an experimental context API. The old API will be supported in all 16.x releases, but applications using it should migrate to the new version. The legacy API will be removed in a future major React versin. Read the [legacy context docs here](/docs/legacy-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.
Typo: version
@@ -6,7 +6,7 @@ permalink: docs/legacy-context.html | |||
|
|||
> Note: | |||
> | |||
> The legacy context API will be removed in version 17. | |||
> The legacy context API will be removed in a future major version. |
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.
Could we move the entire "legacy context" section to its own document and link to it here? I think that would reduce the potential for confusion.
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.
Isn't this the legacy context document? (legacy-context.md
)
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.
Oh lol, my bad :D
content/docs/context.md
Outdated
`embed:context/multiple-contexts.js` | ||
|
||
If two or more context values are often used together, you might want to consider creating your own render prop component that provides both. [React Context Composer](https://github.com/FormidableLabs/react-context-composer/) is a simple library to help with this. |
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.
I would prefer not to recommend that library. It's fine, but it might not align with our future strategy for composing features. Also wary of recommending any one library over another one in the official docs. If we're going to link to any, we should link to more than one.
Also, let's not refer to anything as "simple." A pet peeve of mine :D
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.
Are you okay with leaving just the first sentence?
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.
Or maybe with a custom example that does that (without a library).
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.
Yeah that part is fine
content/docs/context.md
Outdated
## Accessing Context in Lifecycle Methods | ||
|
||
Accessing values from context in lifecycle methods is a relatively common use case. Instead of adding context to every lifecycle method, you just need to pass it as a prop, and then work with it just like you'd normally work with a prop. |
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.
Copied straight from reactjs#587 (comment)
Probably could use some wordsmithing
theme: themes.light, | ||
}; | ||
|
||
this.toggleTheme = () => { |
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.
Note: this is still using property initializer syntax
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.
Isn't arrow fn in the constructor the same as .bind in the 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.
Yes but it's not in any stable standard. It relies on the "public fields" proposal which is in stage 3.
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.
Ooh never mind I completely missed that you did this inside the constructor. I guess it's not very idiomatic (people tend to use bind
instead so that the method is defined together with other class methods), but it's fine.
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.
Will change if you think it's confusing.
https://twitter.com/alexmkrolick/status/977277592087666688
FWIW it's the same in the ref docs: https://github.com/reactjs/reactjs.org/pull/587/files#diff-5ff76597e6c547982df2b2e42f2f83c0R285
WIP again