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

Remove Hydrate in favour of dedicated static templates and reactive objects #206

Open
dphfox opened this issue Feb 1, 2023 · 4 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 1, 2023

Right now, there are two main uses for Hydrate:

  • Cloning from a static template and infusing it with functionality
  • Connecting reactive objects to existing instances to effect changes on properties and children

The second point causes lots of trouble for Fusion conceptually, as it adds two expectations onto Fusion's templating syntax that it was never designed to accomodate; the ability to redefine the same values twice causing underspecified and glitchy results, and the possibility of a template not being valid at all times due to the need for templates to be 'unbindable'.

The solution I propose here is to drop Hydrate completely as a feature, and instead replace it with dedicated solutions for each of these problems on their own.

Firstly, for people who build UIs from templates built externally (usually in Studio), I propose extending the New method to allow cloning an existing instance in place of using a class name. This would have to be implemented in tandem with #124 to allow for child hydration;

local foo = New (ReplicatedStorage.ButtonTemplate) {
    [OnEvent "Activated"] = function()
        print("I was clicked!")
    end
}

This allows for Fusion's prior model (where all instances created by Fusion code are wholly managed by Fusion for the lifetime of the script) to be naturally upheld, as it can be thought of as similar to replacing default properties and adding default children (which would be a new concept but not one that conceptually violates any rules). This makes more sense in the context of a templating syntax compared to the previous idea of mixing managed and unmanaged instances which would invalidate the template as a single source of truth about the instance it describes.

Simultaneously, we should add a suite of state object APIs for 'plugging into' existing instances in a way which can be easily and granularly managed. For example:

local part = ChildOf(workspace, "Part")
local partColour = PropertyOf(foo, "Color")
local light = ChildOfClass(part, "Light")
local unbind = BindProperty(light, "Color", partColour)

-- later
unbind()

This makes much more sense when defining 'flows' of data, which is often where Hydrate is used today outside of templating. The reactive graph is best used for defining data flows, not templating syntax intended for constructing UI. This also unlocks exciting new use cases that were not available before, and brings about opportunities to reduce boilerplate for operations that currently require manual observation to achieve.

There do exist other use cases of Hydrate for such things as allowing users to define arbitrary properties on components, but this is generally not considered to be a good idea as it can leak the internal details of how the component works by turning hidden instance properties into part of the component's own public API. Most often these solutions are intended to reduce the redundant boilerplate of assigning common properties such as Name or Parent, but these use cases would ultimately be better served by dedicated APIs for integrating common sets of properties automatically. Since this is merely boilerplate reduction for convenience, and not what I would consider to be a theoretically sound use case for Hydrate, I'm choosing not to include this in scope of this change. However, we should probably still address this.

@dphfox dphfox added enhancement New feature or request ready to work on Enhancements/changes ready to be made area: roblox labels Feb 1, 2023
@dphfox dphfox added this to Fusion 0.3 Feb 1, 2023
@dphfox dphfox moved this to To Do in Fusion 0.3 Feb 1, 2023
@dphfox dphfox moved this from To Do to Needs Design in Fusion 0.3 Feb 1, 2023
@dphfox dphfox moved this from Needs Design to To Do in Fusion 0.3 Feb 1, 2023
@Aloroid Aloroid mentioned this issue Mar 6, 2023
@dphfox dphfox pinned this issue Aug 17, 2023
@dphfox dphfox unpinned this issue Aug 17, 2023
@dphfox
Copy link
Owner Author

dphfox commented Aug 22, 2023

I propose the following exhaustive list of APIs (all accept state):

Children

  • ChildrenOf(instance)
  • ChildOfName(instance, name)
  • ChildOfClass(instance, className)
  • ChildWhichIsA(instance, className)

Descendants

  • DescendantsOf(instance)
  • DescendantOfName(instance, name)
  • DescendantOfClass(instance, className)
  • DescendantWhichIsA(instance, className)

Ancestors

  • AncestorsOf(instance) (ordered list?)
  • AncestorOfName(instance, name)
  • AncestorOfClass(instance, className)
  • AncestorWhichIsA(instance, className)

Properties

  • PropertyOf(instance, property)
  • BindProperty(instance, property, state) (graph object)

Attributes

  • AttributesOf(instance)
  • AttributeOf(instance, attribute)
  • BindAttribute(instance, attribute, state) (graph object)
  • HasAttribute(instance, property)

Tags

  • TagsOf(instance)
  • BindTags(instance, state) (state returns {string} | string | nil, graph object)
  • HasTag(instance, property)

@dphfox
Copy link
Owner Author

dphfox commented Aug 26, 2023

These APIs should nil-coalesce for composability, so if a child or property along the chain is missing, the error can be picked up at the end. The only place this may cause ambiguity is with instance properties which are nilable, but these are few and far between.

@LoganDark
Copy link

If there are multiple children or descendants with the same name or class, those should probably all be returned, or there should be variants of those APIs that can return multiple instances. Since there's no real way to order them, they could be returned as a hash set (of the form {[Instance]: true}). Then, other adapters like ForKeys could be used to operate on each child.

If the hash-set-returning variants were the canonical variants though, this would raise the cost of obtaining a single child, since you'd have to layer a Computed over it just to call next. And even if that wasn't the case, if you create state objects inside ForKeys at all, you could run into #270. So there would be a couple other things to iron out first.

You could create some set of filtering APIs that allows one to filter DescendantsOf themselves... or you could just do it anyway - I can imagine something like FilterKeys, FilterValues, FilterPairs would be immensely useful for all sorts of reasons, not just this.

Anyway, I'm creating an inventory system and it would have been nice if I could automatically create the proper state objects and event handlers for each inventory slot rather than having to manage all of that manually. This would definitely be a great addition to Fusion.

@LoganDark
Copy link

LoganDark commented Sep 29, 2023

I managed to get something like this working on the latest master, but it's sort of annoying right now, and inhibits garbage collection because there's no way to collect script connections. Still, it may be worth looking at:

local function createDerivedThing<V, C>(
	process: (use: Fusion.Use, refresh: () -> ()) -> {value: V, cleanup: C}?,
	cleanup: (C) -> ()
): Fusion.StateObject<V>
	local updater = Fusion.Value(nil)

	local computed = Fusion.Computed(
		function(use) use(updater) return process(use, function() updater:set(nil, true) end) end,
		function(data) if data then cleanup(data.cleanup) end end
	)

	return Fusion.Computed(function(use)
		local data = use(computed)
		return if data then data.value else nil
	end, Fusion.doNothing)
end

local function childrenOf(value: Fusion.StateObject<Instance?>): Fusion.StateObject<{Instance}?>
	return createDerivedThing( function(use, refresh)
		local instance = use(value) :: Instance?
		if not instance then return nil end

		return {
			value = instance:GetChildren(),
			cleanup = {
				instance.ChildAdded:Connect(refresh),
				instance.ChildRemoved:Connect(refresh)
			}
		}
	end, function(cleanup)
		for _, conn in ipairs(cleanup) do
			conn:Disconnect()
		end
	end)
end

local function childByName(value: Fusion.StateObject<Instance?>, name: Fusion.StateObject<string?>): Fusion.StateObject<Instance?>
	local children = childrenOf(value)

	return createDerivedThing(function(use, refresh)
		local children, name = use(children), use(name)
		if not children or not name then return nil end

		local found: Instance?

		for _, child in ipairs(children) do
			if child.Name == name then
				found = child
			end
		end

		if found then
			local conn = found:GetPropertyChangedSignal('Name'):Connect(refresh)
			return {value = found :: Instance?, cleanup = {conn}}
		else
			local connections = {}

			for i, child in ipairs(children) do
				connections[i] = child:GetPropertyChangedSignal('Name'):Connect(refresh)
			end

			return {value = nil, cleanup = connections}
		end
	end, function(cleanup)
		for _, conn in ipairs(cleanup) do
			conn:Disconnect()
		end
	end)
end

Would be interesting to know if #269 intends to make this more ergonomic or what can really be done about this.

But I need this now, so in lieu of a better solution, I'm using this.

Next major hurdle is #270, which is preventing me from making selectable list items. Time to see if that's possible to fix.

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