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.

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

Closed
wants to merge 1 commit into from
Closed

Conversation

acdlite
Copy link
Collaborator

@acdlite acdlite commented Jul 31, 2018

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

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2018

Note to self: Don’t need to copy the whole list if you prepend. (Edit: Done)

workInProgress: Fiber,
provider: RootContextProvider<any>,
renderExpirationTime: ExpirationTime,
) {
Copy link
Contributor

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.

@acdlite acdlite force-pushed the global-state branch 2 times, most recently from 1b8dab2 to ee57fb3 Compare July 31, 2018 18:30
@sophiebits
Copy link
Collaborator

Should the set API be contextual? In particular I'm thinking of something like:

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.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2018

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

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2018

I suppose the prop in the stateful version of a provider could be named initialValue. Then it's more obvious.

@sophiebits
Copy link
Collaborator

You would need to define a local set yourself, but you could. Something somehow like this:

const ThemeMeta = React.createContext({set: Theme.set});
...
const {set} = ThemeMeta.unstable_read();

Where you could add a ThemeMeta.Provider in Section too with a custom implementation of set (that does a state update or w/e). Exact API probably needs to be better than my proposal here, but basically there would be no way to set without reading that contextually in a way that can be shadowed. I'm not sure if that breaks any other (subscribe to global Flux store, etc) subscription use cases.

@acdlite
Copy link
Collaborator Author

acdlite commented Jul 31, 2018

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.

@acdlite acdlite force-pushed the global-state branch 4 times, most recently from ff5fbaf to 35bbf59 Compare August 1, 2018 20:20
@acdlite
Copy link
Collaborator Author

acdlite commented Aug 1, 2018

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)
Copy link
Collaborator Author

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.

@acdlite acdlite changed the title React.createGlobalState() Context.set(newGlobalValue) Aug 1, 2018
@TrySound
Copy link
Contributor

TrySound commented Aug 1, 2018

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>
)

@gaearon
Copy link
Collaborator

gaearon commented Aug 1, 2018

The set API is only for the top level. Maybe setDefaultValue would be clearer.

@sebmarkbage
Copy link
Collaborator

We've been trying to get away from the confusing naming of set(...) since it really enqueues a change. push(...)?

@TrySound
Copy link
Contributor

TrySound commented Aug 1, 2018

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', () => {
Copy link
Collaborator Author

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.

@acdlite acdlite force-pushed the global-state branch 3 times, most recently from 706aeb9 to 5d5eac0 Compare August 3, 2018 00:22
@alexeyraspopov
Copy link
Contributor

If I click the toggle button, presumably I should override the theme provided by the prop? But then what happens when the prop changes?

@acdlite, what if Provider had onChange property that handles how Context.write() (as opposite to Context.read()) changes the value?

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.)
@acdlite acdlite force-pushed the global-state branch 2 times, most recently from adc093e to 97361e2 Compare August 15, 2018 21:14
@sebmarkbage sebmarkbage mentioned this pull request Aug 16, 2018
12 tasks
Copy link
Collaborator

@sebmarkbage sebmarkbage left a 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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

unstable_write?

Copy link
Contributor

@tjallingt tjallingt Aug 26, 2018

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

Copy link
Collaborator

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.

Copy link
Collaborator

@sebmarkbage sebmarkbage Aug 26, 2018

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,
Copy link
Collaborator

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,
Copy link
Collaborator

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(),
Copy link
Collaborator

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),
Copy link
Collaborator

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.

Copy link
Collaborator

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.

@alexeyraspopov
Copy link
Contributor

@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 Context.set() from the most nested consumer would update closest Provider or some "global value"?

@gaearon
Copy link
Collaborator

gaearon commented Aug 23, 2018

It's always meant to update the global value. Think of it as uppermost level.

@acdlite
Copy link
Collaborator Author

acdlite commented Jan 23, 2020

Will likely revisit this feature in the future. Closing for now.

@acdlite acdlite closed this Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants