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

Move dependency detection into dependency capturers #111

Closed
dphfox opened this issue Dec 28, 2021 · 5 comments · Fixed by #216
Closed

Move dependency detection into dependency capturers #111

dphfox opened this issue Dec 28, 2021 · 5 comments · Fixed by #216
Assignees
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made

Comments

@dphfox
Copy link
Owner

dphfox commented Dec 28, 2021

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 dphfox added enhancement New feature or request not ready - evaluating Currently gauging feedback labels Dec 28, 2021
@Zyrakia
Copy link
Contributor

Zyrakia commented Jan 4, 2022

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.

@dphfox
Copy link
Owner Author

dphfox commented Jan 4, 2022

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 dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - evaluating Currently gauging feedback labels Jan 9, 2022
@dphfox
Copy link
Owner Author

dphfox commented Jan 9, 2022

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 dphfox self-assigned this Jan 29, 2022
@dphfox dphfox moved this to To Do in Fusion 0.3 Jan 29, 2022
@dphfox dphfox moved this from To Do to In Progress in Fusion 0.3 Jan 29, 2022
@dphfox dphfox moved this from In Progress to To Do in Fusion 0.3 Jan 29, 2022
@LoganDark
Copy link

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.

@dphfox
Copy link
Owner Author

dphfox commented Feb 9, 2022

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.

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
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants