Should applyInstanceProps flatten state objects passed to it, or error when given a nested state object?

[dphfox]

This question really comes down to intention versus convenience.

I'm currently evaluating whether to add explicit flattening primitives in #234, but I'm wondering if we should implicitly do this when passing values as properties to New, too.

An argument for this change is that it becomes easier to convert values to state objects without affecting the logic processing them. Consider assigning BackgroundColor3 a Computed that returns a theme colour. That code would currently break if the theme colour was not a constant value. However, if we flattened all state objects, then it can be a state object with no change of function.

However, this could also lead to more silent breaking changes. Returning state objects from state objects could be the result of an error in your logic; you might not have unwrapped something enough times, you might be processing the wrong value, etc. Are these errors still easy to catch if we didn't throw an error when receiving nested state objects? How does the parallel implementation of #234 affect the likelihood of such errors? Since this kind of flattening could implicitly add more dependencies, could this lead people to write code that doesn't follow best practices? Could it encourage writing code in a less performant way? Is it better to make people explicitly flatten things they want flattened?

[Dionysusnu]

Related to #142

[funwolf7]

I personally don't like the idea of it implicitly flattening nested state objects. For me, if there is ever a nested state object being passed to an object, then it is definitely an error on my part.

Consider assigning BackgroundColor3 a Computed that returns a theme colour. That code would currently break if the theme colour was not a constant value. However, if we flattened all state objects, then it can be a state object with no change of function.

If you are using a Computed, you will generally be doing some sort of math or operation on the StateObject, which would already require you to use or flatten (I will refer to the idea in #234 as flatten) the object anyways. The only reason I can think that you would want to return a StateObject in a Computed is if you want to do something like selecting from a list of StateObjects. If this is the case, and you truly want your code to be future-proof, you could just call use or flatten on the chosen StateObject and return the result. As an added bonus, doing so will make it easier for other code to use the Computed, as it ensures that there are no nested StateObjects, removing the need for other code to use flatten or repeated peek calls.

If we don't want to force users to add these calls, an idea would be a function that takes a CanBeState and returns a new StateObject that is a flattened version of it. Ideally, you could specify a maximum depth to search, making it so that any extra nested StateObjects will cause an error.
Something like this (I wrote this just to showcase the idea, the name and method of implementation could be changed):

local function cloneUnfolded(target: Fusion.CanBeState<any>, maximumDepth: number?, destructor: ((any) -> ())?): Fusion.Computed<any>
	return Computed(function(use)
		local currentTarget = target
		local currentDepth = 0

		while true do
			if xtypeof(currentTarget) ~= "State" or (maximumDepth and currentDepth >= maximumDepth) then
				return currentTarget
			end

			currentTarget = use(currentTarget)
			currentDepth += 1
		end
	end, destructor)
end

An example usage:

local nestedBackgroundColor = Value(Value(Value(Color3.fromRGB(255, 0, 0))))
local frame = New "Frame" {
	BackgroundColor3 = cloneUnfolded(nestedBackgroundColor),
} :: Frame
task.wait()
print(frame.BackgroundColor3) --> 1, 0, 0
peek(peek(nestedBackgroundColor)):set(Color3.fromRGB(0, 255, 0))
task.wait()
print(frame.BackgroundColor3) --> 0, 1, 0
peek(nestedBackgroundColor):set(Color3.fromRGB(0, 0, 255))
task.wait()
print(frame.BackgroundColor3) --> 0, 0, 1

Ultimately, both of these methods tell both the code and readers that the nested StateObjects are intentional, making code clearer and less error-prone. I don't believe we should allow for silent bugs simply so we can have a bit of convenience.

[dphfox]

I agree that this probably isn't a good direction to pursue.