-
Notifications
You must be signed in to change notification settings - Fork 76
feat(StateStore): state modifiers and store merging #1461
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
Conversation
ff47a28
to
5f4f942
Compare
5f4f942
to
9995196
Compare
Size Change: +5.08 kB (+1.24%) Total Size: 414 kB
|
7e34139
to
6d565a1
Compare
6d565a1
to
5408ab3
Compare
originalValue !== this.cachedOriginalValue || | ||
mergedValue !== this.cachedMergedValue | ||
) { | ||
this.value = { |
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.
Do we want to give a try to utils/mergeWith.ts? It could be handy for nested objects like message composer config is
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 don't think this will be needed as the mechanism does only a shallow merge due to potential type conflicts, see this part which dissalows merging two stores if they have top-level keys in common.
const storeA = new StateStore({ a: 1, b: 2 });
const storeB = new StateStore({ x: 1, b: 'aa' });
storeA.merge(storeB); // type error -> both stores have `b` state property
If this was not in place, the state would produce unexpected results whenever either of the states changed.
If storeA
updates b
to, let's say 10
- the merged store would look like this: { a: 1, x: 1, b: 10 }
while the merged type would look like this: { a: number; x: number; b: string | number }
. Subscribers with selector pointing to b
would receive 10
. Then storeB
updates b
to hello
now subscribers with selector poiting to b
would receive hello
and suddenly there's no value/type stability - this does not seem that bad until you decide to get latest value and you need b
value to be a number but since storeA
hasn't changed since, you'll get hello
instead.
Potential solution to this problem is to have the actual merged state object look like this:
type MergedStateStoreState<X, Y> = {
original: StateStore<X>;
merged: StateStore<Y>;
}
But even in this form there's no need for mergeWith
just yet.
5408ab3
to
efb6d00
Compare
4ded36f
to
1f5de53
Compare
Description of the changes, What, Why and How?