-
Notifications
You must be signed in to change notification settings - Fork 55
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
A better way to orchestrate multiple stores #146
Comments
Building upon an internal solution of ours, we developed a simple way as a "workaround" to the problem. First for some context, we "wrap" all our containers with a higher order container from the beginning. This is the pattern we are already using; const counter = new WeakMap();
export const createWrappedContainer = (
store,
options
) => {
const OriginalContainer = createContainer(store, {
...options,
onInit: () => (api, containerProps) => {
const current = counter.get(api.getState) || 0;
counter.set(api.getState, current + 1);
options?.onInit?.()(api, containerProps);
},
onCleanup: () => (api, containerProps) => {
const current = counter.get(api.getState);
counter.set(api.getState, current - 1);
// This is the workaround for https://github.com/atlassian/react-sweet-state/issues/97
if (current - 1 === 0) {
// Do the cleanup for your common actions. e.g we cancel all pending requests if any
}
options?.onCleanup?.()(api, containerProps);
},
});
const HOC: typeof OriginalContainer = ({ ...props }) => {
// Place external hooks that need access from all containers here
const exampleValue = useExampleHook();
return (
<Consumer>
{(consumerProps) => {
return (
<OriginalContainer
{...props}
__injectedProp1__={consumerProps.valueA}
__injectedProp2__={exampleValue}
></OriginalContainer>
);
}}
</Consumer>
);
};
return HOC;
}; All our stores use this "wrapped" container creator, which already limits our usage of container-less stores. The mechanism is not strictly important but it may also be a hook (see Building upon our wrapper, we considered adding a export const createWrappedContainer = (
store,
options
) => {
const OriginalContainer = createContainer(store, {
...options,
// Other arrangements...
});
const HookRunner = () => {
// This is not against rules of hooks as it is deterministically initialized once
// Here we can access this store instance as well as others higher in the tree
// It is important to consider the update cost when doing something here
// Other options pass down container props, store values, but it gets complicated very quickly.
options?.containerHook?.();
return null;
};
const HOC: typeof OriginalContainer = ({ ...props }) => {
// Other arrangements
return (
<OriginalContainer {...props}>
<HookRunner />
{props.children}
</OriginalContainer>
);
};
return HOC;
}; With this we are able to solve #131 #139 and the issues described above. Consider the usage; const useCustomHook = () => {
// We are able to use any store hook here and access their instance next up in the tree
// It is now trivial to compare values from previous renders, just utilize React hooks API:
const [storeAState,] = useStoreA(); // This is our data
const [,storeBActions] = useStoreB(); // Can access any store
useEffect(() => {
storeBActions.doAction(storeAState.id);
}, [storeAState.id]); // Hooks keep track of our data already!
// Other alternatives include getting original container props or store state/actions as hook params
};
const MyAwesomeContainer = createWrappedContainer(storeA, {
containerHook: useCustomHook
}) Then instead of this added complexity and many decisions ahead (what to pass down as props etc.), we realized that a simple API change on RSS (or our API in that matter) would make things much easier; const StoreAContainer = createContainer(storeA, {
// This is React element compatible such that we can just use a standard component
// But the children is actually the rendered original RSS container
wrapper: ({children}) => {
// Wrap with any value provider here be it a consumer or hook
<Consumer>
{(consumerProps) => {
return (
React.cloneElement(children, {
children.props,
// inject any other container props here
}, ...children.children)
);
}}
</Consumer>
}
}); Here, we follow the standard wrapper API where We will implement this alternative |
We have created the functionality on our wrapper and we are testing it to make sure it is useful in the way we intended. Unfortunately, our RSS usage has dropped a little so it is taking some while for us to properly battle test this. Once we are happy though, I am planning to open a PR for a very simple API change that opens a new set of tooling that will solve most of the problems discussed above. To give a general idea, when you need to access another store, our container definitions look like this; export const MyContainer = createContainer(myStore, {
// This is the only API (`Wrapper`) that we need to introduce. `createDefaultWrapper` is
// just an opinionated helper.
Wrapper: createDefaultWrapper({
useSyncContainerProps: () => {
const flagApi = useFlag(); // This could as well be from another RSS store
// Note that if the above hook update, the whole container will re-render.
// This is intended though, b/c we want to use the fresh values in this tree.
// So when using other RSS stores, it is important to only subscribe to what you need.
// These will get injected to the container, so that every instance of MyContainer can access them
return {
flagApi,
};
},
useContainerHook: () => {
const [{stateA}] = useExternalStore();
const [state, actions] = useMyStore();
// No need for juggling the prev state etc. just use react hooks to sync stuff to
// all instances of the store. This makes the synchronization very local and explicit.
// This store depends on a state from the external store and it is clearly defined here.
useEffect(() => {
actions.update(stateA);
}, [stateA]);
},
}),
}); Note that we only add a This is function InnerElement({
children,
useContainerHook,
containerProps,
}) {
useContainerHook?.(containerProps);
return children;
}
export function createDefaultWrapper({
useSyncContainerProps,
useContainerHook,
}) {
return ({ children }) => {
if (React.isValidElement(children)) {
const containerProps = useSyncContainerProps
? useSyncContainerProps(children.props);
return React.cloneElement(
children,
{ ...children.props, ...containerProps },
useContainerHook ? (
<InnerElement
useContainerHook={useContainerHook}
containerProps={containerProps}
>
{children.props.children}
</InnerElement>
) : (
children.props.children
),
);
}
return null;
};
} |
One of the main premises of react-sweet-state is separating things into multiple more manageable stores, thus it is important to keep individual store's state small both for readability and performance reasons. This brings a few rough corners that I think we should find a solution.
Using data from another store requires us to hop through
store hook
->container props (jsx)
->action
.data store
to make a decision in thelist store
's actions we need to go through that chain.Using actions from another store requires us to hop through
store hook
->jsx
(optionally ->container props
->action
).onSubmit={() => storeAAction().then(storeBAction)}
. Which makes it hard to understand the overall flow.Because the containers decide on which data to use it is probably not very straightforward to solve this but something like this would be great if ever possible;
This clearly couples the
list store
to thedata store
but this is not necessarily a bad thing. We need that store/data to execute this separate flow, while still preventingdata store
's users from updating when the order changes for a local part of the UI somewhere.data store
updates should cause this store to update as well though.This is something that will get out the hands of the user on the other hand, b/c now there is no mechanism to decide when to update
list store
depending on the data that changed on thedata store
. We might also need something to do the container's update action in this case. This somewhat relates to #131.I think this need also relates to #139 overall.
Edit: We even had a case where we needed to cross two different stores to share a piece of information a few days ago.
The text was updated successfully, but these errors were encountered: