dphfox / Fusion

A modern reactive UI library, built specifically for Roblox and Luau.
https://elttob.uk/Fusion/
MIT License
592 stars 100 forks source link

'Flatten' state objects #234

Open dphfox opened 1 year ago

dphfox commented 1 year ago

Right now it can be awkward to work with nested state objects, especially if you don't know how deeply nested they will be, because in order to correctly access the value you need, you also have to manage a lot of observers to make sure you catch when one of the state objects at any level changes value.

We should probably implement this as a new kind of state object, which can take in any existing state object and output the innermost value obtained by unwrapping as many times as possible (or perhaps a configurable number of unwraps).

dphfox commented 1 year ago

It's worth noting that in 0.3, it's less cumbersome to do thanks to #35. You can (finitely) unwrap something using nested use calls:

local parcel = Value(Value(Value("toothbrush")))
local unwrapped = Computed(function(use)
    -- unwraps up to five layers
    return use(use(use(use(use(parcel)))))
end)
print(peek(unwrapped)) --> toothbrush

You could perhaps unwrap an indefinite number of layers using a loop; this is definitely not ergonomic though.

Dionysusnu commented 1 year ago

Relates to #142

dphfox commented 1 year ago

Ah yes. I think it'll be worth addressing the points I made previously there. For clarification, that previous issue related to implicit flattening in :get(), and in applyInstanceProps. The latter relates to #235, while the former relates more to this proposal.

This is a large complication - When Fusion detects a state object being passed in, right now it just sets up an observer and applies changes to the values as they come in. Making this behaviour recursive increases overhead for new learners trying to understand how Fusion works, and has the potential to massively increase the complexity of Fusion under the hood, as multiple observers will have to be created and juggled in what is one of the most critical parts of the API.

I still maintain that changing use/peek to flatten state objects would be far too heavy handed. I think it's about 50% true for applyInstanceProps, so I'm open to being convinced there one way or the other. Explicit flattening makes this an opt-in operation, so any complication is entirely opt-in. This works better in my mind.

This obscures the true shape of code - I do not think a double :get() is ugly in the slightest. Instead, I think it is an honest reflection of the structure which this code is dealing with. It quickly and effectively communicates what we expect position to contain - a nested object - and there is very little room for misinterpretation here. If we recursively evaluated state objects like this, we could be encouraging users to write code that is more writable yet less readable, by encouraging users to write code that doesn't reflect the structure that it operates upon. Writability is heavily discounted throughout Fusion's design and readability is emphasised, so I do not like the implications of this change.

I think this is true, and another reason why explicit flattening works better.

This assumes intent where intent is not guaranteed - there are two sides to this coin. We may be making it easier to intentionally accept nested values, but we would also be making it much, much easier to unintentionally unwrap nested values too. If we made :get() recurse, then how do you specify what you actually want Fusion to unwrap? What about if something in your code goes wrong and you accidentally pass in a nested state object - could this wreak havoc on a codebase silently? This will necessarily increase the overhead of working with :get() by adding considerations that could extend to every single use of :get(). It also requires weakening the strict behaviour of state objects in general, leading to code that is harder to reason about and harder to make guarantees about.

Still true. Explicit flattening removes the intent problem. This is why I'm iffy about applyInstanceProps flattening implicitly.

This puts extra work on third party library developers - We intentionally want to keep standardised interfaces as simple to implement as reasonably possible, so that we can encourage the development of third-party libraries that duplicate as little functionality as possible and which behave as predictably as possible. By keeping the definition of :get() simple, essentially acting as a pure getter, we impose almost no burden on third party library developers to get that behaviour working correctly and reliably. If we extended the specification of :get() to mandate recursive unwrapping, we would now be asking all third party library developers to implement this specifically defined behaviour exactly to spec. This is a bad approach - if we need something done consistently across all projects, Fusion should implement it on it's own rather than pushing it onto a spec.

Explicit flattening works with all state objects by default.

This would have deep implications internally - :get() is meant as the standard, universal way to get the value of a state object directly. It's designed like this to decouple the inner function of a state object from Fusion entirely. A lot of Fusion code depends on this fact internally. If we were to start modifying the behaviour of this highly fundamental API surface, we would have to consider not only how this will change the behaviour of internal APIs, but also whether we need to surface new parameters, and how would be the best way of approaching that if so. This is not a simple change to make, and so we would need very good and crystal clear motivation to change this. This would also have implications on some big upcoming paradigm shifts like use/unwrap, which were designed with the express intent of removing all special functionality from :get().

Explicit flattening makes this a non-issue, as introducing explicit flattening does not change existing behaviour.

dphfox commented 1 year ago

Design should be simple enough for this. Either we could introduce a peek() or use()-style callback, or alternatively a Flatten state object. Probably not both.

dphfox commented 8 months ago

This can be trivially implemented as a function;

local function flatten<T>(
    use: Use,
    thing: CanBeState<T>
): T
    while isState(thing) do
        thing = use(thing)
    end
    return thing
end

I suggest we go for this function implementation first, and only create a full state object if it becomes a common pattern.

dphfox commented 8 months ago

Upgrading the importance of this feature request because #301 may increase the frequency of code that needs to deal with an unknown level of state object nesting.