-
Notifications
You must be signed in to change notification settings - Fork 558
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
createSharedState #130
base: main
Are you sure you want to change the base?
createSharedState #130
Conversation
Related: This RFC reminds me of one of the possibilities in the Alternatives section of the Context.write RFC. |
Just noticed you have already discovered that along with many others. 🙈 It's meant more like a persistence across browser tabs which alone is a very nice feature. Although as it uses localStorage, the reference integrity for anything more than primitive value is lost. That can cause fairly haywire situations with effects. That said, I probably don't see any added value of having this in core API since it's not that hard to create in user-land. On the contrary, the |
Let me share my motivation behind adding it to core API. (Also added to the summary section):
|
Well, I still think you are missing the important point here. The Context true power lies in the Provider, where you can "override" the value based on its position in the tree. The Error Boundary, Suspense and probably more to come are building on that base premise that any component can look up higher in the tree to find closest relevant value without influencing even higher positioned elements. Look at the Furthermore, the Provider is essential for unit/integration testing. Dealing with any sort of top scoped variables and resetting those properly can be fairly hard. Currently, there are already too many options on how to track the state. I don't think it's right way adding another one. In my opinion, it would only fracture the community even more. |
I do not see much gain in this "feature". A main feature of context is, that it is composable, that you can have multiple context providers in your app and the closest one is in effect. You can implement this kind of singleton state you are proposing already very easily with the tools React has already. |
I Like the concept, though I would be worried about performance issues in Enterprise size applications. I don't agree that the verbosity of the provider vs the simplicity of this pattern should be ignored because Context has more features nor would this be hard to test imo Sometimes less is more :) |
It is easy to maintained in a package independently. We have done quite similar work since hooks was published. And we have published a package named |
|
||
# Motivation | ||
|
||
`createSharedState` results with a cleaner codebase with easily reusable shared state. Using this function will eliminated not necessary re-renders caused by top-level `Provider` in case of syncing data between nested components on different branches of component 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.
If I understand your statement correctly, I think it is inaccurate. Just because a higher-level component re-renders does not mean that all its children will necessarily re-render. https://kentcdodds.com/blog/optimize-react-re-renders
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.
You can optimize for sure but without any manual intervention everything below provider will be rerendered.
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.
@mucsi96, wheh a Provider changes value, children are not re-rendered, unless they are subscribed to the same context.
Demo: https://codesandbox.io/s/pedantic-moore-cxfq5
|
||
# Drawbacks | ||
|
||
As with any React hooks this works only in functional components. |
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 would create a singleton which has the drawback of not working very well in SSR environments. I think that's a ship stopper personally.
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 expect in SSR environment the initial value to be rendered. Can you explain please what is the issue with SSR environments?
I like createSharedState, and I also recommend concent, it is more than shared state, you can have a try. |
export function createSharedState(defaultValue) { | ||
const listeners = new Set(); | ||
let backupValue = defaultValue; | ||
|
||
return () => { | ||
const [value, setValue] = useState(backupValue); | ||
|
||
useEffect(() => { | ||
listeners.add(setValue); | ||
return () => listeners.delete(setValue); | ||
}, []); | ||
|
||
useEffect(() => { | ||
backupValue = value; | ||
listeners.forEach(listener => listener !== setValue && listener(value)); | ||
}, [value]); | ||
|
||
return [value, setValue]; | ||
}; | ||
} |
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've done this in an internal project at work, and I highlight recommend doing these changes:
- Keep the setter separate, it allows you use to use it:
- for setup, outside components.
- in class components.
- in function component without "hooking" onto the state, so if you only use the setter, it won't trigger a rerender for that component on state changes.
- in tests, to reset the state for example.
- Update the state immediately in the
useEffect
, as it might have been updated after the render, and before the effect call.
export function createSharedState(defaultValue) { | |
const listeners = new Set(); | |
let backupValue = defaultValue; | |
return () => { | |
const [value, setValue] = useState(backupValue); | |
useEffect(() => { | |
listeners.add(setValue); | |
return () => listeners.delete(setValue); | |
}, []); | |
useEffect(() => { | |
backupValue = value; | |
listeners.forEach(listener => listener !== setValue && listener(value)); | |
}, [value]); | |
return [value, setValue]; | |
}; | |
} | |
export function createSharedState(initialState) { | |
const listeners = new Set(); | |
let sharedState = initialState; | |
function useSharedState() { | |
const [state, setState] = useState(sharedState); | |
useEffect(() => { | |
listeners.add(setState); | |
setState(sharedState); | |
return () => { | |
listeners.delete(setState); | |
}; | |
}, []); | |
return state; | |
} | |
function setSharedState(newState) { | |
if (typeof newState === 'function') { | |
newState = newState(sharedState); | |
} | |
if (!Object.is(sharedState, newState)) { | |
sharedState = newState; | |
for (const listener of listeners) { | |
listener(sharedState); | |
} | |
} | |
} | |
return [useSharedState, setSharedState]; | |
} |
I think this use case is covered by Recoil, right? |
sounds like my new RFC makes this one redundant, please check it out #179 also, I am still trying to figure out if the RFC mechanism is active, or is it just a time waster for the community, because It seems like no one from the Core team is active here. |
@Niryo You made a similar comment on an RFC issue earlier this week. No need to post it on multiple issues. The team reads every RFC and generally finds RFC submissions very helpful. |
@bvaughn , yep I have made it 5 days ago, and didn't got any response until now:) (and the owner already closed the PR so I thought my comment will be lost). |
I have to agree. I think no response is the biggest killer of open source collaboration. It really demotivates from any future contributions. |
@Niryo I don't think this kind of meta discussion belongs on an RFC. (Each of these comments notifies people who are subscribed to this issue– but they aren't related to the issue.) I'll respond briefly, (because I don't have any other way to contact you), but ask that this is the last meta comment on this issue. Sometimes RFCs that seem "easy" don't align with the longer term vision of React. For example, the RFC you commented on earlier this week was suggesting a change to the class component lifecycles to make async data fetching easier, but this was filed at a time when the team was thinking about using the Suspense API for data fetching and moving away from class components in general to function components (and hooks) so that it would be easier for application code to be concurrent mode compatible. Sometimes RFCs have good ideas that we want to act on, but it's not clear how to best incorporate them with our other efforts so they fit into the larger release plan we're working toward. Maybe we need to do a better job of communicating this scenario. I hear you that it's frustrating to write an RFC and not get much of a response. I'll share that with the team (as I shared your comment from a few days ago with the team). One thing to keep in mind though is that the React team is small and it's pretty difficult to keep up with GitHub issues (especially RFCs). I think we do read all of the RFcs though, for what it's worth, and many of them inform longer term decisions. |
In the spirit of #182, I'm making a pass to comment on the RFCs even in cases where we don't have fully fleshed out thinking. So I'll do my best to comment on this one. I think the problem is fair. It's a general tradeoff of React: we tend to do things that are composable, occasionally at the cost of the "simple" use case. E.g. context is composable. You can take an app using written Context, then render this app twice (left and right), and each copy will keep working side by side. They won't interfere. But with this approach, they would. Someone could write a component using "shared state" because it's just less boilerplate. Someone else would use that state in another component. So far, so good. Eventually there's no clear data flow because there's no clear ownership. You want to reuse one of those components in a different place (with slightly different behavior), but its data spills into other components in a way that wasn't intended. Then you remember: oh... so that's why we had unidirectional data flow. Of course, this is harder to appreciate in a small example. And it's true that sometimes you actually do want some pieces of state to always be in sync. So I'm not dismissing this outright. It's just that we need to be very careful about where and why we encourage this. On a high level, we don't really think "global state" is a very compelling concept by itself. Rather, we would divide it into two (possibly more) separate cases. Some of what's considered "global state" is really UI state. UI state, even when not very convenient, should have ownership and lifetime. We think it should be tied to a specific part of the tree. Even if that part is at a very high level. This ensures that you can keep composing and reusing components without them accidentally synchronizing in weird ways. However, not all state is like this. Some truly feels "global" — like fetched data. Fetched data is not UI state. But we don't think about it as state at all. We think about data as a cache. You're essentially "reading from the network". A cache only exists temporarily to make it faster. You should be able to throw it away, and the app should be able to recover. So it's a different case. It's not actually stateful. We're working on a built-in Cache for data (reactwg/react-18#25). To give you an idea of what it might feel like to use it, here's a few examples. That's separate from the topic of this RFC — but since people often talk about "state" but mean "data", I wanted to mention our strategy is different for those cases. Then there is "local data" which is where this proposal usually shines. Thought it's still debatable that it should be modeled as "global state". Often you'll later get requirements that it should persist between sessions. Or maybe sync between tabs. Or maybe sync to the server... So the question of ownership and lifetime and when exactly it gets "synced", if at all, is not super clear-cut. One thing that I think a "shared state"-like API needs to consider is what is the migration path when you do want to isolate it. Like in Recoil, if I implement a todo lists with atoms, what do I do if I want to render two separate todo lists side by side? Do I rewrite my app? There is a RecoilRoot which offers some isolation, but this might be too strong (e.g. maybe I want to isolate todo list themselves but keep some data inside them shared). There are also features like atom families which appear to be related. So any proposal in this vein, IMO, needs to have a good story about how it scales up when the isolation has become necessary. This RFC does not seem to dive into this question. Overall, we try not to bring something into core unless it's (1) impossible in userland, or (2) very beneficial to adopt. In this case I think (1) is not true so far, and (2) is debatable. It's beneficial in some cases but poses a unique set of challenges that still require more research. So we don't have a resolution on this yet, but more discussion is always welcome. |
I have a similar implementation, followed by useSyncExternalStore. import type { Dispatch, SetStateAction } from 'react';
import { useCallback, useDebugValue } from 'react';
import { useSyncExternalStore } from 'use-sync-external-store/shim';
import { SharedState } from './sharedState';
export function createSharedState<Snapshot = undefined>(): <
Selection = undefined,
>(
selector?: (state: Snapshot) => Selection,
) => [
(Selection extends undefined ? Snapshot : Selection) | undefined,
Dispatch<SetStateAction<Snapshot | undefined>>,
];
export function createSharedState<Snapshot>(
initialState: Snapshot | (() => Snapshot),
): <Selection = undefined>(
selector?: (state: Snapshot) => Selection,
) => [
Selection extends undefined ? Snapshot : Selection,
Dispatch<SetStateAction<Snapshot>>,
];
export function createSharedState(initialState?: any) {
let sharedState: SharedState<any>;
const useSharedState = (selector?: (state: any) => any) => {
if (!sharedState) {
sharedState = new SharedState(initialState);
}
const getSnapshot = useCallback(
() => sharedState.getState(selector ? selector : (state) => state),
[],
);
const state = useSyncExternalStore(
sharedState.subscribe,
getSnapshot,
getSnapshot,
);
useDebugValue(state);
return [state, sharedState.setState];
};
return useSharedState;
} |
Summary
I would like to add a single function which allows sharing data between components with hooks. This could be used instead of Context API when scoping or cascading to single component structure branch is not needed. In many cases this leads to more simple implementation compared to using Context API.
createSharedState
creates a hook very similar touseState
but with sync state across hook instances. Setting a value in one component will result re-rendering every component which uses the same hook. The state is preserved even if all hooks become unmounted.Motivation
createSharedState
results with a cleaner code-base with easily reusable shared state. Using this function will eliminated not necessary re-renders caused by top-levelProvider
in case of syncing data between nested components on different branches of component tree.In React community many projects are already created with very similar idea:
Even though the solution can be easily created in user-land my motivation is the following to add it to core API:
Side-by-side comparison with Context API
Basic example
I suggest having an API such as this:
CodeSandbox
NPM package
Full RFC text rendered