Feature: Custom Resettable Attribute #98
Conversation
|
|
||
| if let cache = lookupCache(of: atom, for: key) { | ||
| let context = AtomCurrentContext(store: self, coordinator: state.coordinator) | ||
| atom.reset(context: context) |
There was a problem hiding this comment.
@ra1028
The most important part for review, as there are multiple potential options.
Since reset itself is a void function without side-effects by itself, but also can have a lot of them depending on what is being called (operations affecting the store & cache). As a result, its possible that it will have serious side effects and multiple downstream updates, as well as possible to have none.
This suggested implementation has following key characteristics (that can be discussed)
-
It executes regardless of
overridestatus. This is because code should not care about if atom is overriden or not when it comes to reset, given that it might be expecting key side-effect handling or state change, and override (which is commonly utilized in tests) would break that contract and make environment not clean. -
It executes before new cache generation and update (not after). Im not convinced myself if this is a proper approach here, and would like to get your input. Reason I chose this way is simply because if reset logic has important data handling that depend on current state of the system, they can be lost if atom is reset and all subscribers are notified. But that as well might be expected otherwise sometimes.. Third option would be not to generate new cache / update at all.. In that case, if atom value is not watching anything, then it effectively disables reset ability to initial state. I sort of considered using
_loader.shouldUpdateto check whether there were changes to atom or not when deciding if to make new cache or not, butshouldUpdatealways return true except in modified/select atoms.
There was a problem hiding this comment.
It executes regardless of override status
It's not necessary to take into account the override status. It's kind of a compromise, but Refreshable also calls the atom's custom refresh regardless of the atom is overridden or not.
It executes before new cache generation and update.
Rather, do not create make a new cache after calling the custom reset. Ressetable should allow atoms to have arbitrary "reset" behavior. If you reset the dependencies of the atom, it re-creates the atom value transitively, if not, then it simply calls arbitrary reset behavior.
There was a problem hiding this comment.
It's kind of a compromise, but
Refreshablealso calls the atom's custom refresh regardless of the atom is overridden or not.
if let override {
value = override.value(atom)
}
else {
let context = AtomCurrentContext(store: self, coordinator: state.coordinator)
value = await atom.refresh(context: context)
}@ra1028 What am I missing here www
There was a problem hiding this comment.
Oh sorry, that was my slight misunderstanding. More accurately, the custom refresh isn't performed when it's overridden and just returns the overridden value.
This means that custom behavior by attributes is not fully supported when overridden, so reset doesn't really need to care about it for now.
There was a problem hiding this comment.
However, come to think of it, refresh may also better follow how reset works that calls the custom reset regardless it's overridden 🤔
README.md
Outdated
| ```swift | ||
| struct RandomIntAtom: ValueAtom, Resettable, Hashable { | ||
| func value(context: Context) -> Int { | ||
| let generator = context.watch(RandomNumberGeneratorAtom()) |
There was a problem hiding this comment.
Not tested, but tried to expand existing example not to introduce completely new context. Used this as reference
| } | ||
|
|
||
| let store = AtomStore() | ||
| let transaction = Transaction(key: AtomKey(transactionAtom)) {} |
There was a problem hiding this comment.
This confuses me, need some irrelevant atom or test fails.. Transaction concept is difficult to grasp
rasberik
changed the title
| /// let context = ... | ||
| /// print(context.watch(TextAtom())) // Prints "Text" | ||
| /// context[TextAtom()] = "New text" | ||
| /// print(context.read(TextAtom())) // Prints "New text" | ||
| /// context.reset(TextAtom()) | ||
| /// print(context.read(TextAtom())) // Prints "Text" |
There was a problem hiding this comment.
I think those example code should be a bit different from the original reset but I have no idea of good example 🤔
There was a problem hiding this comment.
Hard make good example without writing long atom describing code. Updated but its still not very appealing, let me know what you think
|
|
||
| if let cache = lookupCache(of: atom, for: key) { | ||
| let context = AtomCurrentContext(store: self, coordinator: state.coordinator) | ||
| atom.reset(context: context) |
There was a problem hiding this comment.
It executes regardless of override status
It's not necessary to take into account the override status. It's kind of a compromise, but Refreshable also calls the atom's custom refresh regardless of the atom is overridden or not.
It executes before new cache generation and update.
Rather, do not create make a new cache after calling the custom reset. Ressetable should allow atoms to have arbitrary "reset" behavior. If you reset the dependencies of the atom, it re-creates the atom value transitively, if not, then it simply calls arbitrary reset behavior.
Pull Request Type
Description
Add
Resettableattribute that allows an atom to have a reset callback