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

Add something like "ComputedInstanceProperty" #134

Closed
LoganDark opened this issue Feb 9, 2022 · 5 comments
Closed

Add something like "ComputedInstanceProperty" #134

LoganDark opened this issue Feb 9, 2022 · 5 comments
Labels
enhancement New feature or request

Comments

@LoganDark
Copy link

I'm making a Tool and I'd like to use Fusion for its state management capabilities. It's just really nice for building up a dependency graph for the final "can this tool fire" state, one that won't ever error or fail due to unexpected edge cases.

One of my requirements for the tool firing is that the character's Humanoid must be alive. This sounds simple but is actually surprisingly nuanced for two main reasons:

  • The Humanoid changes over time as the tool is un/equipped, and dropped then picked up by different players. It may be nil at any time, and may change any number of times.
  • The Humanoid.Health property is not itself a state object, so you can't just put humanoid:get().Health in a Computed.

As described in #128, I have made a function that solves both of these problems:

local function instanceDerivedProperty(source: Fusion.StateObject<Instance?>, property: string): Fusion.Computed<any?>
	local state = Fusion.Value(nil :: any?)
	local changedConn: RBXScriptConnection? = nil

	local function onStateChanged()
		if changedConn then
			changedConn:Disconnect()
			changedConn = nil
		end

		local newInstance = source:get()

		if newInstance then
			local myConn: RBXScriptConnection -- Deferred events ruining the fun for everyone
			local newInstanceAsTable = (newInstance :: any) :: {[string]: any}

			local function onPropChanged()
				if changedConn == myConn then
					state:set(newInstanceAsTable[property])
				end
			end

			myConn = newInstance:GetPropertyChangedSignal(property):Connect(onPropChanged)
			changedConn = myConn

			onPropChanged()
		else
			state:set(nil)
		end
	end

	Fusion.Observer(source):onChange(onStateChanged)
	onStateChanged()

	return Fusion.Computed(function()
		return state:get()
	end)
end

local humanoidHealth = instanceDerivedProperty(humanoid :: any, 'Health') :: Fusion.Computed<number?>

I was waiting on feedback from @Elttob but as they haven't replied in a couple days and I've gotten a green-ish light from @nottirb to go ahead and make this a full feature request, I'm going to go ahead with this anyway.

This feature request is intended to track the possible inclusion of such a utility in Fusion so that tracking properties of an instance in state requires less boilerplate. It follows the trend of ComputedPairs/ComputedKeys/ComputedValues functions that take a state, extract something from whatever is inside, and properly update whenever that dependency does.

Please note that while the above function is technically sound as far as I can tell, it will most likely be implemented differently in Fusion so this should not be considered any sort of code proposal. It's only included for illustrative purposes, since it doesn't work perfectly with Luau's type system.

@LoganDark LoganDark added enhancement New feature or request not ready - evaluating Currently gauging feedback labels Feb 9, 2022
@dphfox
Copy link
Owner

dphfox commented Feb 9, 2022

Sorry for the lack of response - I read the notification on my phone a couple days ago but never acted on it.

I've been thinking about this though, and I think the real root issue here is that it's difficult to change what your UI hydrates. I'm not keen at all on introducing overly specific helpers - about the furthest I'm comfortable with is in the ballpark of things like ForEach. We should be building generally useful tools, not hyper-specialised helpers. The latter does not typically belong in the core library, and is instead better suited to third party libraries.

I have many conflicting thoughts about being able to redirect where you're hydrating to:

  • We already don't allow you to do something similar with New(className)(props) - that doesn't accept state objects because 1) if you need to dynamically change class type or the property table at runtime, this is a code smell, and 2) changing class type would require complete reconstruction of the instance, breaking the 'one New, one instance' paradigm and introducing all sorts of horrible side effects with things like Ref.
  • Along a similar vein, I don't know if it's a good idea to allow for Hydrate to allow a state object for the instance. This relates to some discussion in Match children by name #124 where we're talking about selectors. In general, my opinion is currently 'one instance should have one property table' and therefore 'no instances should share the same property table'. This leaves one detail up for grabs - does this apply temporally? In other words, is this rule about instances existing simultaneously, or is it a stricter 1:1 mapping where an instance shouldn't ever share it's property table anywhere else, even after use?

It's worth noting that in Fusion v0.2 the code already becomes partially syntactically simplified thanks to Hydrate and Out (though it is worth noting there isn't yet a notion of 'un-hydrating' - this is something I'm thinking about how best to address, but for now it renders this code pretty impractical and memory leaky):

local function InstanceDerivedProperty(source, property)
    local currentProp = Value()

	local function updateSource()
		-- TODO: unbind this in the future
		Hydrate(source:get()) {
			[Out(property)] = currentProp
		}
	end

	Observer(source):onChange(updateSource)
	updateSource()

	return Computed(function()
		return currentProp:get()
	end)
end

Of course, being able to redirect Hydrate solves this use case perfectly, but it raises questions about whether it's actually a net positive for code quality:

local function InstanceDerivedProperty(source, property)
	local currentProp = Value()

	Hydrate(source) {
		[Out(property)] = currentProp
	}

	return Computed(function()
		return currentProp:get()
	end)
end

This is not something I'm prepared to answer right now.

@LoganDark
Copy link
Author

LoganDark commented Feb 9, 2022

Of course, being able to redirect Hydrate solves this use case perfectly, but it raises questions about whether it's actually a net positive for code quality:

Using Hydrate to make an existing interface functional and using Hydrate just to abuse Out props feel like two entirely different concepts to me and in only the latter case would you ever want to redirect the target of the props. If you really want to start talking root causes and building blocks then I think it's worth considering those different concepts and how they could be reshaped to fit better and support use cases like this one more naturally.

Hydrate isn't what I'm looking for, I don't think.

@Dionysusnu
Copy link
Contributor

Kind of a duplicate of #47 but it seems it's being reconsidered.

@LoganDark
Copy link
Author

Kind of a duplicate of #47 but it seems it's being reconsidered.

I think the difference is that the particular situation this issue is concerning can't really be solved by hydration (in a non-memory-leaky/unsound way) because the instance I want the property of is contained in state and may change.

@dphfox
Copy link
Owner

dphfox commented Feb 1, 2023

Merging with #206

@dphfox dphfox closed this as completed Feb 1, 2023
@dphfox dphfox added status: rejected and removed not ready - evaluating Currently gauging feedback labels Feb 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants