Closed bluenote10 closed 4 years ago
Hmm, I would not consider this a bug - handling updates could be done with spreading previous part of the state
setState({"a": {...state.a, "x": 42}})
I'll see what does TS think of this constructs when I get the time to make the PR about improving type safety of setState.
My understanding of making the API support partials is that it is more efficient. Ideally both example above should see that a.y
hasn't changed and avoid triggering corresponding computations. I'm not sure if that would be the case with spreading. Does Solid run a diff on individual properties or will it think all properties of a
have changed?
If nested levels don't handle partials properly that's a bug. I have tests for that. Objects should merge. I considered other behavior but it was I felt the cleanest way to update multiple distinct properties at the same time. It also supports spreading changes distinctly that way. It does a shallow eqality check on each property during merge. The only part a bit weird is using objects to update array indexes say to swap rows.
I should mention I don't support dot path. You need to make a separate argument per path variable. String splitting is absurdly slow.
I will check in any case but I'm a bit surprised Objects aren't merging nested. I use it nested one level down in the JS Frameworks Bench even to do swap rows.
Oh, I see the difference... you need to use array paths. I don't do deep merging of objects. In that sense, it's the same as React. you need the ability to replace objects. I only do a shallow merge on objects.
So to your examples to achieve what you are after looks like this:
setState('a', 'x', 42) // direct set, or
setState('a', {x: 42}) // merge set
These are equivalent and merge like you expect. You can do an arbitrarily long path and only the last is value. These behaviours hold for function form too. It basically means spreading on the previous state is unnecessary since it's always a merge. The only tricky part is you have to go up a level if you want to do a replace. To replace a
and not merge it you'd have to write this:
setState({a: {x: 42}})
Does that make sense?
Yes sorry I got the string based example wrong, I'm lost without compiler help ;).
What I was trying to suggest is to extend the functionality of the type-safe object based updates so that they can do everything the string based updates can do.
Let's assume we add an optional boolean argument replace
(possibly defaulting to false
). This would enable the API to achieve both:
setState({a: {b: {c: 42}}})
setState({a: {b: {c: 42}}}, true)
The first statement would fully merge i.e. only sets a.b.c
to 42 exactly like the equivalent string based update.
The second statement replaces the object at a.b
. Replacement would always happen at the last level, i.e. the first two levels don't get replaced so that paths like a.bb
or aa.b
won't be removed.
This would cover the two main use cases imho, but I'm not yet sure if it makes sense.
Hmmm... given that form starts with a plain object it might be serviceable as I can detect it differently. Although I think shallow merging should be the default behavior not the other way around to be consistent with objects along paths.
I considered a syntax like that at one point (see React Immutable Helpers). They used operators to distinguish update behavior. See the thing that kept me from it is being able to control the point of replace is really important in mutable structures so forcing to the leaves doesn't cover important cases. I was worried it would be confusing why.
Now this might be based on the type of data I'm working with. A lot of models and ids. Picture swapping models. There is a big difference from moving the object reference and rewriting each property with the other models. It breaks referential keyed list reconciliation etc. I debated an always replace approach but I ultimately thought shallow merge the most beneficial for even being basically able codify mutations almost declaratively(almost like a monad). It is consistent with React and makes it easy to mutate multiple properties at once(same benefits with deep merge)
Along that vein path syntax allows writing batch changes without iteration using ranges and filters, and even multiple distinct updates in the same setState
call. I did this to reduce the need for people to write everything in freeze
. action
in MobX is a bit irksome from that perspective.
That being said for simple updates I can see deep being useful. To date, I would just make a custom reconciler like setState(deep(obj))
. But I think this a decent point to take new information on and reconsider this a bit. Especially now that custom reconcilers are growing (already have reconcile
and force
). The truth is there seem to be a number of different behavior intents when updating state. While I want to keep the default light syntax, maybe I need to look at the means I have of injecting different behavior.
Thanks for sharing your thoughts. I now think my proposal with the boolean doesn't work that well. For instance:
setState({a: someObject}, true)
The intention of such code is to replace a
. But if someObject
itself contains nested paths the rule of "replace at deepest level" would lead to confusing behavior.
What makes more sense to me now is to make deep merging the default and introduce a wrapper replace
for marking certain paths for replacement:
// Replaces entire state
setState(replace({a: {b: {c: 42}}}))
// Replaces state.a
setState({a: replace({b: {c: 42}})})
// Replaces state.a.b
setState({a: {b: replace({c: 42})}})
I guess that is just the inverse of the suggested deep
? An opt-out of merging looks more flexible to me, because a single setState
could be used to merge on outer levels and mark different subpaths for replacement. I'm not sure if this would work the other way around (if an outer level has replace semantics, there is no point in marking an inner level for merging).
What you are suggesting is essentially the aforementioned Immutable Helper's API (https://github.com/kolodny/immutability-helper). They use $ keywords but more or less the same. I understand why given the shape resembles the original form this would be preferable for TypeScript, which was not part of my original consideration. Although I did play with these variations for a few months.
The reason I liked what I ended up with is that at a base level it requires no extra syntax for the common cases. And then it extends in a scalable way (consistent with depth) to handles the majority of cases. I suppose I could go through a set of examples, but it also is the most flexible while keeping the tersest syntax. Nothing complicated just the scenarios you'd hit in a simple app that managed a list. JS Framework Benchmark has some great examples. The use of Arrays with objects removes the need for extra syntax to denote when the intention changes. Falcor's paths(https://netflix.github.io/falcor/documentation/paths.html) gave me the solution I was looking for in able to descriptively handle ranges.
I think the challenge here is these approaches are mutually exclusive if I set deepMerge as the default as it would require an API change that diverges from paths. Whereas with the current method, this deepMerge behaviour could be opt-in. The paths also support cases there aren't equivalents to in the deep merge.
What I was more reconsidering is the syntax for swapping out behaviour. Right now it's a wrap. Maybe if setState always forced a single argument, where the second was an optional custom behaviour, it would be clearer, and a nicer way to handle options. Your true
as an option might be the basis. Like:
// morphs old state to new state enacting the right fine-grained changes
setState({a: {b: {c: 42}}}, reconcile({ key: 'id' }));
// replaces 'a' and notifies even if value hasn't changed
setState({a: {b: {c: 42}}}, force);
// only updates the leaves
setState({a: {b: {c: 42}}}, deepMerge)
Of course, it means paths would always have to be an array like:
// single path
setState(['count', 1])
// multi path
setState([
['count', 1]
['data', 3, 'hello']
])
The only thing I don't like about is it adds more to the common simple single path case which I'm not sure I find acceptable. It is slightly more explicit. But it becomes a thing. It could be argued that path behaviour could also be opt-in, leaving the default React equivalent, but that isn't a complete solution. This area could benefit from more thought.
if I set deepMerge as the default as it would require an API change that diverges from paths
In my opinion object based vs path based updates are semantically different enough that this consistency is rather confusing. With path based updates, I already specify a path, so replace semantics is what I'd expect intuitively. With object based updates we always start at the root, so naturally one rather expects merge behavior. That's why I expected the following to behave the same (even you seemed surprised by lack of merging):
setState("a", "x", 42)
setState({"a": {"x": 42}})
Regarding your comment on why you decided against wrappers:
See the thing that kept me from it is being able to control the point of replace is really important in mutable structures so forcing to the leaves doesn't cover important cases. I was worried it would be confusing why.
I totally agree that the point of replacement is crucial. But isn't that exactly the benefit of wrappers because they would clearly specify the point of replacement? To me it's much more confusing to have different behavior on the first level compared to nested level.
(I noticed this problem when refactoring some state from top level into a sub-object. The compiler helped to identify all usages that I had to adjust, and yet my app suddenly stopped working at runtime. I didn't expect setState to change its behavior from such a refactoring; not so easy to debug as a beginner.)
Regarding wrapper vs a secondary argument. When changing my mind from the boolean to wrappers I went through a few similar ideas that encode the update behavior in the secondary argument. In the end I preferred wrapper because of (1) locality of information and (2) possibility to encode multiple things at once. For instance, consider if need a mixed update with replacement at different levels and some plain merging:
setState({
a: replace(newA),
b: {sub: replace(newSub)},
x: {y: {z: 42}},
})
With a secondary argument we either have to split this into multiple setStates or come up with an awkward way of referring to specific data paths from the secondary argument.
Re-reading I realize I did a poor job of explaining myself coherently. I will try to remedy that.
That's why I expected the following to behave the same (even you seemed surprised by lack of merging)
That was all my misunderstanding on my part of what you were saying. When you said top-level I thought you meant the first item in the path not the depth of the object. I didn't understand what the issue was at first as it didn't occur to me what you were expecting to happen.
The reason is something I refer to these days as "React Blinders". It comes up often but I tend to expect people to have used React before which saves me from explaining stuff since at a basic level I try to imitate React directly. More than once recently this has come up to bite me a bit. If you ignore everything else Solid's setState
works like React's. This is very intentional. I didn't want any confusion with the similarity of APIs. It's the level one for someone who is coming in. Ie.. this should replace 'a' exactly as React does:
setState({a: {x: 42}})
Level 2 was just recognizing that any path in front behaves the same when it hits an object to be consistent. No matter the depth of path this works consistently. It still replaces 'a':
setState('parent', 'nested', {a: {x: 42}})
I thought you were saying that this was replacing 'nested'.
See the thing that kept me from it is being able to control the point of replace is really important in mutable structures so forcing to the leaves doesn't cover important cases. I was worried it would be confusing why.
But isn't that exactly the benefit of wrappers because they would clearly specify the point of replacement?
Yes. I meant that any approach that merged deep by default would need special wrappers for common cases and failing to use them (perhaps because you didn't know) would be confusing to explain. Again this might be coming from my "React Blinders" you'd think you could just setState
the same way.
Mostly, I think it's awkward for the general behaviour to require additional imports or syntax. I really wanted to avoid that. Most similar libraries don't try to wrap with an immutable API. They let you just use Proxy setters so I understand why this seems a bit unusual. But the current solution assuming React knowledge is consistent as well for simple cases (Level1, Level2). Where it gets a little hairier is Level3 where you are performing multiple updates in a single setState
:
setState({
a: replace(newA),
b: {sub: replace(newSub)},
x: {y: {z: 42}},
})
Is this
setState([
{a: newA}
['b', {sub: newSub}]
['x', 'y', 'z', 42},
])
It does bring up a decent point though. While combining multiple states is an easy refactor as previous states all just need another entry in the path in front, mixed refactoring is awkward.
Ok, let's remove my "React Blinders" for a second and try this again. And run through some reasoning:
reconcile
behaviour). That way the state is replaced but only updates are made to things that actually changed.This is basically where I was sitting when I realized adopting React conventions could work simply and consistently. But let's explore a different explicit take. All updates follow a progression even if you skip by them to the end:
Paths -> Path -> Merge -> Set(replace)
At some point, you are setting a value. It may be multiple values in proximity due to a merge. There may be a Path or range pattern to getting to the Merge and Set/s points. It is possible you want to perform multiple of these updates(Paths) synchronously. Paths -> Path is trivial, use an Array. If we don't want objects to mean anything on there own we need a merge
operator to go from path to merge and a set
to go from merge object to set point. If we assume array at root are paths and objects are merge, and multiple arguments is path, we can have reasonable starting points.
Unfortunately, these operators need access to the original state object so either we need to pass that in explicitly or bind them ahead of time since we don't want to require the state getter to be transported along with setState`. This leaves a few options. Don't use real operators and develop a syntax like immutable helpers:
setState({
a: {$set: newA},
b: {sub: {$set: newSub}},
x: {y: {z: 42}},
})
setState(
'user',
'$merge',
{
firstName: 'John',
address: {$set: newAddress}
}
)
Use operators but get them from createState
some how
// maybe 3rd option
const [state, setState, {set, merge}] = createState();
// or hang them off setState
const [state, setState] = createState();
// setState.set, setState.merge
setState({
a: set(newA),
b: {sub: set(newSub)},
x: {y: {z: 42}},
})
setState(
'user',
merge({
firstName: 'John',
address: set(newAddress)
})
)
I like the look of this better. It would be easy to pass other parameters into merge or set to change their behaviour.
Another option is to pass it through options on the function form:
setState((prevState, { set }) => ({
a: set(newA),
b: {sub: set(newSub)},
x: {y: {z: 42}},
}))
setState(
'user',
(prevUser, { merge, set }) => merge({
firstName: 'John',
address: set(newAddress)
})
)
Not obvious but cleaner still. I actually don't hate this one. The one downside of all of these is that tree-shaking is not an option. Merge and set mind you are simple enough I suppose that isn't a big deal. But how to indicate more complicated reconciliation would need to be different. Hmm.. can we do this without binding ahead?
Ok, new thought what if we can pass state in by under the hood generating the function form. Ie;
function set(newObj) {
return (prevState) => { /* ... do stuff * / }
}
Sure, should work, and is tree-shakeable. Hmm.. now I need to import these every time now. set
and merge
are too general for imports so need probably a more descriptive update. Not sure I like this for default actions. Stuff like reconcile
could benefit from being tree-shakeable but set/merge should be built-in.
Ok, so now we have some solution options. They just seem complicated when compared to something like React for common cases. Like a top-level update of objects would need sets across the board. It's about as awkward as doing it with paths. I'm still not sure deep merge is generalizable enough in an easy way. However, I do feel there is some interesting stuff from this exploration. I want to process this a bit and see if there is some potential improvement here. And of course, any thoughts about this thinking are welcome.
I was just shown https://github.com/kube/monolite by @friedroman. I think there might be something here that could make everyone happy.
Hmm.. looking through this more there seems to be one solution that basically handles all my use cases. While something like monolite is cool and using passing nested setter functions could work, I realized I was fighting with it a bit since this since Solid's underbelly is mutable. Not that we couldn't provide an interface like monolite. But it occurs to the core problem is simpler than that and I believe I have a solution. And I apologize if I shut down the mobx/immer folk perhaps prematurely. Because while I dislike the pattern, having that underpinning gives us an escape hatch to build behaviours on top and I believe I can simplify a lot of things.
So I'm proposing the following (keep in mind I haven't completely vetted this):
state[property] = someValue
. Otherwise state cannot be mutated this way. It is only available inside the function setter of setState
. ie setState(state => /*...*/);
. Benefit here of course is TypeScript should work in this zone.setState
works more or less the way it works now at a base. Essentially setState(...path, setter)
. Simple values, arrays. Objects shallow merge. These can be set directly or set via a function setter. With one exception. If the returned value if the the same reference to state then we do not replace/merge, we just leave as unchanged. This is consistent with immutability. However because we have opened up mutability it just means the system assumes you have taken care of your own mutation. What this means is you can essentially use setState
exactly the same as today. Although I believe we should remove the multi path form (more on that below).reconcile
and force
. But now you could nest it. Ie. setState('users', 0, reconcile(updatedUser));
or setState('users', 0, u => reconcile(updatedUser)(u));
. The second form is important since you could:
setState('users', users => {
users
.filter(u => u.address === oldAddress)
.forEach(u => reconcile(newAddress)(u.address))
return users;
})
Not that we would necessarily do that exact example with a reconcile
. But what I'm getting at is you can do multi-update branching very easily. But do notice I return the original users object instead of mapping to a new one. Setting a new one would say the whole list needs to be updated. Whereas what I have written above just mutates the address on the user. Not even the user itself.
Ok so what does this mean well. We could add a paths
helper if we wanted. Then we could achieve the current multipath behavior like so setState(paths([...], [...], [...]))
or nested setState('data', paths(/**/))
. If you want to use something like monolite well we could.
While I very much dislike opening this up from an API standpoint, it is a really straight forward way to allow any state updating pattern one would want to use. While I personally never ever want do this:
setState(s => {
s.users[2].firstName = 'Jake';
s.users[2].lastName= 'Smith';
return s;
});
And instead do this:
setState('users', 2, {firstName: 'Jake', lastName: 'Smith'});
I am leaving it open. Maybe you use TypeScript. Maybe we make a typedPath
helper:
setState(typedPath(s => s.users[2], {firstName: 'Jake', lastName: 'Smith'}));
I like that much better than the imperative example but it is this approach that makes this straightforward to do. Maybe you like deep object merging:
setState(deep({users: {2: {firstName: 'Jake', lastName: 'Smith'}}}));
There you go. Maybe you have your own custom API:
setState(custom({users: {2: {$merge: {firstName: 'Jake', lastName: 'Smith'}}}));
Anyway, looking for thoughts or feedback. Maybe I should write up a separate RFC.
@ryansolid I personally like the immer
type approach, it is the easiest and most approachable to anyone using solid
for the first time.
What are your reasons for not liking it ?
My reasons for liking it:
$merge
(from immutability-helper
)setState
methodYeah it's just number 4. It opens up room for procedural code. I also like the succintness of paths.
Technically we could already use freeze
to wrap multiple setState
commands. Initially I was looking for a way to codify mutations. But in practice I gave up on that.. I made something very similar to Redux that was based on mutators instead of reducers where you passed around codified mutations. I think similar to the idea of monads. But in the end I just ended up using redux with reconcile
or using [state, {...actions}]
Context API stores. So arguably it's not that important.
If you understand my proposal I'm suggesting that the function form of setState
works like immer. So basically I can keep my path syntax and allow the immer syntax top level or nested. With that capability we can support pretty much any pattern with the use of optional helpers, you can always fallback to doing it immer style. It actually makes the helpers more general purpose. You can pretty much ignore path syntax exists for your purposes.
Hmm.. actually I see one difference from immer. They don't expect you to return anything. Where I'm proposing you have to return the reference. That is interesting. We could support that. It just means that function form could never be used to set a value to undefined. Not an issue top-level of course. I used this trick to identifier custom reconcilers. It just would mean that setState('count', () => undefined);
would be a no op. I could live with that. I think the more awkward question is what would you expect this to do setState(s => s.name = 'John');
Technically that function returns 'John'. It isn't enough to handle undefineds.
This path forces functional form to always be mutations. I could live with that but it's very breaking. Right down to the core example:
setState('count', c => c + 1);
Of course I could make exceptions for basic types, but you could never do:
setState('list', l => [..l, item]);
and always have to do:
setState('list', l => l.push(item));
Although I guess technically you could:
setState('list', [..state.list, item]);
Hmm.. I need to think about that. Any other thoughts?
EDIT: nvm https://immerjs.github.io/immer/docs/return.. So return does matter. Ok this can work. That's pretty exciting. What is really cool about all of this that people might not realize is since our internals are mutations not immutable the performance is significantly better than ImmutableJS or Immer.
I'm new, so maybe I missed a discussion somewhere, but why do we need setState
? Why not just an API like
import { createState, onCleanup } from "solid-js";
const CountingComponent = () => {
const state = createState({ counter: 0 });
const interval = setInterval(() => {
state.counter += 1 // <----------- this, right here
}, 1000);
onCleanup(() => clearInterval(interval));
return <div>{state.counter}</div>;
};
I guess more concretely, why do we have to do
setState(state => state.counter += 1)
instead of just
state.counter += 1
?
@trusktr Are you familiar with Actions and strict mode in MobX? This is the same motivation. When you have a reactive system unhinged from components the danger of a web of mess and unexpected cycles becomes huge. I've worked in giant production KnockoutJS systems I wanted to avoid this at all costs. This is why even strayed from a proxy mutable API like Immer until this point, but that atleast has wrapper.
The following is going to be a bit opinionated but it comes down to the foundation of why I built this library.
It really comes down to uni-directional flows. I built Solid off those same principles as React. I strongly believe in explicit setters and that even with Proxies you can avoid the Vue/Svelte issues of losing the meaning of an assignment. Proxies are dangerous when setting a value can trigger a whole bunch of reactions. I like having explicit control. Simply passing state down the tree doesn't give the child the right to modify it. 2-way binding is a terrible pattern and while it provides ease of use it can cause immense chaos. I talk about this a bit in a couple of my articles but I still stand by: What Every JavaScript Framework Could Learn from React. This is a good read too: Designing SolidJS: Reactivity
It is the reason why every read and write is split etc.. Why I make my state immutable surface area. It is a big part of what makes Solid unique for a reactive library and comes from my experience(and opinion) of dealing with patterns in these libraries for years.
Actually one of the only reasons I haven't released this yet is it currently works like MobX with Strict Mode on. I want to make sure I enforce it better. In the version I've committed someone can do this:
const [s, setS] = createState({});
const [s2, setS2] = createState({});
setS(() => { s2.name = "Jo"; });
This is pretty terrible. I want it that you can only modify the state that comes from the setter function. So I should use a different specialized proxy that only comes from the setter. I just need to figure out how to keep code side down by reusing the right parts and not have it affect performance in the general case.
Ok I've now implemented a different proxy for read vs writes. This keeps things well guarded. The only thing to consider is proxies are not equal even when they proxy the same underlying object. So you want to do all conditions and comparisons outside of the setter since read proxy !=== write proxy.
Anyone else have thoughts @bluenote10 @high1 @friedroman? I'm pretty close to being ready to release.
Hm, my only concern here is that this will be somewhat like React hooks - their API is clean and nice, but you need to know how their instrinsics work.
Thanks for your thoughts. I did have that concern too. We are sort of stuck in that the system is mutable. So it's either direct access or via path unless we want to introduce a bunch of operators. Since full deep merging will not work which could allow us to do immutable cloning. The point of insertion matters.
I think the real problem here is that there might be too many ways to do stuff. There is mutate in a closure, there are paths, and combinations of both being used in either a mutation or an immutable way. These all can work simultaneously. Accidentally falling into a different pattern is a real risk. If you use state one way you will probably never need to know anything specific. It's only if you use it different ways you might get things crossed. I will think if there is anything I can do about it. Immer actually has this aspect too, but they don't also support paths. That gives another dimension.
I'm sort of conflicted because I prefer path syntax and its declarative syntax, and am pushed this way mostly for TypeScript. However once offered this mutable form is undeniably the base API since it can handle all scenarios cleanest. So I almost have to use it as a starting point and reason from there. Which can still expand to what I'm proposing as it is non-conflicting, but it makes me wonder the proper way to introduce the topic.
I'm going to take this time as a period to explore this API and see what makes sense. I've released this now with Solid v0.14.0 and updated the docs. You may have noticed I made all the tests TS as well so I could confirm this. Seems to be working like a charm. I'm going to have to do a little work to see if I can optimize more. The one thing with relying on Proxy triggers especially with Arrays is that a simple action can trigger the setter being called a lot. Like if you unshift
and item in the front of a list it triggers the setter trap for every row in the array. Since Solid's array handling is based on reconciliation it is fine since the whole array ends up being essentially a single reactive node it is worth pointing out that more property accesses happen against the proxy on native method calls than you might realize and I will be looking to reduce any impact of those.
This is probably too much detail for most people. But I just wanted to mention any place where I can make further improvements.
It's been a couple weeks with no activity. I'm going to close this one.
Follow up discussion to #62.
I'm wondering if the type safe implementation of
setState
could be extended to handle nested updates. For example:Unfortunately the two don't behave the same:
state.a.x
leaving everything else untouched.b
untouched, but wipes out they
from the nested object.This feels like a bug to me, because according to the type definitions
setState
takes nested partials, whereas the implementation only handles partials at the first level -- nested levels apparently do not support partial objects as one would expect.@ryansolid Would it make sense to "merge" objects not only at the first level, but also on nested levels as the type definitions suggest? This would leave the question how to fully replace/delete an object. Maybe we would need an optional second argument
dontMerge
so that we can callsetState({"a": {}}, true)
to disable merging and fully replacea
.If that's not a possibility, we may want to change the type definitions to bring them in line with the implementation: Only the outer argument should be a
Partial<T>
, the nested values have to be realT
.