Skip to content
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

'Flatten' state objects #234

Open
dphfox opened this issue Feb 13, 2023 · 6 comments
Open

'Flatten' state objects #234

dphfox opened this issue Feb 13, 2023 · 6 comments
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made

Comments

@dphfox
Copy link
Owner

dphfox commented Feb 13, 2023

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 dphfox added enhancement New feature or request not ready - evaluating Currently gauging feedback area: state labels Feb 13, 2023
@dphfox dphfox moved this to Prospective in Fusion 0.3 Feb 13, 2023
@dphfox
Copy link
Owner Author

dphfox commented Feb 13, 2023

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
Copy link
Contributor

Relates to #142

@dphfox
Copy link
Owner Author

dphfox commented Feb 23, 2023

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 dphfox added not ready - design wanted Good idea but needs design work and removed not ready - evaluating Currently gauging feedback labels Aug 22, 2023
@dphfox
Copy link
Owner Author

dphfox commented Aug 22, 2023

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
Copy link
Owner Author

dphfox commented Feb 4, 2024

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 dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - design wanted Good idea but needs design work labels Feb 4, 2024
@dphfox dphfox moved this from Prospective to To Do in Fusion 0.3 Feb 4, 2024
@dphfox
Copy link
Owner Author

dphfox commented Feb 5, 2024

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.

@dphfox dphfox removed this from Fusion 0.3 Jul 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made
Projects
None yet
Development

No branches or pull requests

2 participants