Fusion icon indicating copy to clipboard operation
Fusion copied to clipboard

Move dependency detection into dependency capturers

Open dphfox opened this issue 3 years ago • 5 comments

Currently, state objects are contractually required to disclose dependencies as part of their :get() process:

-- Computed.lua, for example
function class:get(asDependency: boolean?): any
	if asDependency ~= false then
		useDependency(self)
	end
	return self._value
end

This is.. somewhat ok. It's a bit of of a conflation of two separate responsibilities, but ultimately it's always been a compromise to make user code cleaner while still supporting our dynamic dependency system. This has a nasty tendency to magically introduce dependencies where perhaps the user didn't want them, however - this was a motivation to add the secret asDependency argument for internal Fusion use, which is a quick and dirty hack around the issue. It was never meant to be long term.

One potential way out here is to place the burden of detecting dependencies on the specific code that needs to capture them, rather than engineering a broad dependency detection system that attempts to cater for every single capturing context.

An example of this idea in action might be introducing a use utility to be passed into Computed, for the purposes of declaring dependencies manually - for convenience, it could return the unwrapped value (perhaps similar to how #35 might?):

local x = Value(5)

local doubleX = Computed(function(use)
    return use(x) * 2
end)

Computed could then cut out a lot of potentially costly middle-men and directly save these dependencies into itself.

dphfox avatar Dec 28 '21 02:12 dphfox

I actually think the opposite, when working with components, a lot of people already have their own makeState/statify implementation, just because it's the easiest way to support constants and state and maintain the reactivity and flexibility we all love. This would be a replacement for that, with the exact same amount of code, if not less, and even benefits from things like an official implementation and explicit dependencies that have the 'just write it' feel of implicit dependencies. I'm all for this change.

Zyrakia avatar Jan 04 '22 05:01 Zyrakia

I think fusion should strive to be the cleanest UI library. This approach adds more clutter than enhancement.

Can you clarify on why you think this? As-is, I don't follow your thought train.

dphfox avatar Jan 04 '22 10:01 dphfox

I've been using this privately for a while and I'm confident that it's the right approach forwards now. I'll slate this to replace automatic dependencies in v0.3 I think 🙂

dphfox avatar Jan 09 '22 21:01 dphfox

I love that Fusion automatically detects what values you depend on and subscribes you to them. I think it would detract from the niceness of the library to throw that away and require you to manually notify Computed of what you use.

Perhaps you can support both approaches at once? Instead of an undocumented/"hacky" function argument, you could just allow a field access to get the value at this point in time without declaring a dependency. Alternatively, if that's not feasible (i.e. you don't want people accidentally trying to assign to the field), you could provide another method like :current().

imho, implementing this change as-is this would clutter computed functions a fair bit compared to :get(), especially when you do actually want to depend on every state variable you access. It's subtle but I prefer ...:get() instead of use(...), the latter looks a bit messier to me.

LoganDark avatar Feb 06 '22 11:02 LoganDark

The problem with the current approach - as evidenced by the very common visits to my inbox - is that the current implicit system makes it far too easy to introduce unwanted dependencies at a distance.

I think that it's much better to have an API where dependencies are opt-in, not opt-out, because it makes sure that you're always in control of your UI's updates. It's also much more obvious when something doesn't update correctly, versus when something updates needlessly; the latter issues are hard to identify and diagnose, and cause unintuitive behaviour because the lifecycle of things in your scripts don't match your internal model.

I've done my absolute best to keep the API surface similarly concise - to the point we're actually one character shorter than before - because dependency management is somewhat of a critical path for Fusion. Every single Fusion app has to deal with it throughout, so it's important to keep it writable.

dphfox avatar Feb 09 '22 20:02 dphfox