-
Notifications
You must be signed in to change notification settings - Fork 47.5k
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.set(newGlobalValue) #13293
Context.set(newGlobalValue) #13293
Conversation
Note to self: Don’t need to copy the whole list if you prepend. (Edit: Done) |
workInProgress: Fiber, | ||
provider: RootContextProvider<any>, | ||
renderExpirationTime: ExpirationTime, | ||
) { |
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.
Might be add boolean
return type is fine.
1b8dab2
to
ee57fb3
Compare
Should the function Page() {
return <>
<ToggleTheme />
<Section />
<Section />
<Section />
</>;
}
function Section() {
return <Paragraph />;
}
function Paragraph() {
const theme = Theme.unstable_read();
return <div className={theme}>
...
</div>;
}
function ToggleTheme() {
const theme = Theme.unstable_read();
const handleClick = () => Theme.set(theme === 'dark' ? 'light' : 'dark');
return <input type="button" onClick={handleClick} />;
} If you want to instead store a separate theme per section, you add a Provider to Section and move ToggleTheme to inside it. But you also need to change the implementation of ToggleTheme. What if the Theme.set function, instead of being global, was also passed through context? Then you could shadow it in Section and not need to change ToggleTheme at all for this. This could reduce the potential cost of incorrectly choosing global context in an early rendition of your app. |
@sophiebits I don't think it's obvious what the semantics would be in that case: <Provider value={this.props.theme}>
<ToggleTheme />
<Provider> If I click the toggle button, presumably I should override the theme provided by the prop? But then what happens when the prop changes? |
I suppose the prop in the stateful version of a provider could be named |
You would need to define a local
Where you could add a ThemeMeta.Provider in Section too with a custom implementation of |
The plan for simple-cache-provider was to use two separate contexts: one for the thing you read from, and a separate one for the thing you use to update. |
ff5fbaf
to
35bbf59
Compare
Ok, updated to add the method to the existing context API instead of a creating a new one. |
⚛ AllLifecycles.componentWillUnmount | ||
⚛ (Calling Lifecycle Methods: 0 Total) | ||
⚛ (Calling Lifecycle Methods: 1 Total) |
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.
@sebmarkbage This change is because I'm using a callback to remove the root from the global list. What we should do instead is add an explicit reconciler API for unmounting a root. We had this at one point but now that we're not relying solely on GC we need to add it back. I can do this in a follow up.
Cool! I like this guys. Looks cleaner than render prop and less api for react. Did you consider default value based on props? сonst State = React.createContext(0)
const Component = () => (
<State.Provider>
<div>{State.read()}</div>
<button onClick={() => State.set(State.read() + 1)}>
increment
</button>
</State.Provider>
) |
The |
We've been trying to get away from the confusing naming of |
Or maybe "read" mirror "write"? |
@@ -291,6 +291,47 @@ describe('ReactNewContext', () => { | |||
expect(ReactNoop.getChildren()).toEqual([span('Result: 12')]); | |||
}); | |||
|
|||
it('a change in an ancestor provider does not update consumers within a nested provider', () => { |
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 test passes in master, but it covers a case that wasn't previously covered.
706aeb9
to
5d5eac0
Compare
@acdlite, what if |
Adds a method to update a context's default (global) value. This schedules an update on all context consumers that are not nested inside a provider, across all roots and renderers. For most use cases, this replaces the need to inject a context provider at the top of each root. (I've added an `unstable_` prefix until it's ready for public release.)
adc093e
to
97361e2
Compare
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'm a bit concerned about the contexts Map. All the information is already in the updateQueue. I'm not convinced we need to also store it in the state. Will continue review tomorrow.
unstable_read: () => T, | ||
unstable_set: (value: T, callback: (() => mixed) | void | null) => void, |
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.
unstable_write
?
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.
writeDefault
(or maybe writeGlobal
/ writeBase
)
Context.write()
implies to me like it would write to the nearest parent provider (just like, if i'm not terribly mistaken, Context.read
reads from the nearest parent provider).
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 agree read
/ write
mismatch is confusing.
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.
read
is not going to be a public API anyway so we can rename it to something more random. Or even remove it.
pushDefault
or writeDefault
could work too.
isMounted: boolean, | ||
setContext<T>( | ||
context: ReactContext<T>, | ||
oldVvalue: T, |
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.
oldVvalue
-> oldValue
.
import type {ReactContext} from 'shared/ReactTypes'; | ||
|
||
type GlobalRoot = { | ||
isMounted: boolean, |
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 seems to be unused.
}; | ||
uninitializedFiber.stateNode = root; | ||
uninitializedFiber.memoizedState = { | ||
element: null, | ||
contexts: new Map(), |
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'm not sure we need this Map when the data is already in the update queue but do we need to eagerly initialize this whole thing?
|
||
nextGlobalRoot: null, | ||
previousGlobalRoot: lastGlobalRoot, | ||
setContext: (setRootContext.bind(null, uninitializedFiber): any), |
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.
If we pass the GlobalRoot as an argument to setContext we don't have to bind this function and create a new one for each root. We can share one function per renderer. We can get the fiber from arg.current.
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.
Come to think of it, we already do pass it to this
. So you won't need to bind this function.
@acdlite, what's expected behavior of this method being called in the tree with more than one Provider of a single context used? For example, if a single Provider is used on different levels with different values, calling |
It's always meant to update the global value. Think of it as uppermost level. |
Will likely revisit this feature in the future. Closing for now. |
Adds a method to update a context's default (global) value. This schedules an update on all context consumers that are not nested inside a provider, across all roots and renderers. For most use cases, this replaces the need to inject a context provider at the top of each root.
(I've added an
unstable_
prefix until it's ready for public release.)