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

Binding to existing instances #34

Closed
Anaminus opened this issue Aug 28, 2021 · 17 comments · Fixed by #120
Closed

Binding to existing instances #34

Anaminus opened this issue Aug 28, 2021 · 17 comments · Fixed by #120
Labels
enhancement New feature or request ready to work on Enhancements/changes ready to be made

Comments

@Anaminus
Copy link
Contributor

New allows binding State and whatnot to an instance, but the instance must be created from scratch. There are a handful of useful instances that either already exist, or must be constructed by means other than Instance.new. Such instances should be supported by Fusion. Some off the top of my head:

  • DockWidgetPluginGui
  • PluginAction
  • PluginMenu
  • PluginToolbar
  • PluginToolbarButton
  • Services (probably niche)

One option is to let New (or an additional New-like function) receive an instance instead of a class name. e.g.

New (instance) {
	...
}

Something to consider is passing in instances already known by Fusion. I'm not yet familiar with the internals, so I'm not sure how references are handled here. As for behavior, elements could be merged, with newer elements overriding previous ones. This might also be used to remove bindings.

@dphfox dphfox added the enhancement New feature or request label Aug 29, 2021
@dphfox
Copy link
Owner

dphfox commented Aug 30, 2021

This is definitely a use case I've identified, though I haven't yet got an API design for this use case that I'd be fully confident in.

I probably wouldn't add this functionality onto the New function, since the New function is meant to create instances. It'd probably get too confusing if it started taking on the responsibility of modifying existing instances too.

An API design I experimented with during prototyping was a separate function, essentially the same as what you wrote above, but separate from New:

local message = State("Hello, world")

Hydrate(PlayerGui.ScreenGui.TextLabel) {
    Text = message
}

This could certainly be done! However, I'm unsure if it's the right way to go. Since Fusion build UI instances from code, all of it's patterns are optimised for that way of working. For example:

-- when dealing with children, the syntax is clean and your code has strong visual hierachy
local gui = New "Thing" {
    [Children] = New "Thing" {
        [Children] = New "Thing" {...}
    }
}

-- components are trivial and fall naturally out of the API design
local function Component(props)
    return New "Thing" {...}
end

local gui = New "Thing" {
    [Children] = Component {
        [Children] = New "Thing" {...}
    }
}

What do these constructs look like if you're not creating instances, but instead attaching code to an existing hierachy?

-- dealing with children is much more verbose and lacks the same visual structure
local gui = PlayerGui.MyGui
Hydrate(gui) {
    Value = ...
}
Hydrate(gui.Thing) {
    Value = ...
}
Hydrate(gui.Thing.OtherThing) {
    Value = ...
}

-- components get awkward
local function Component(instance)
    return function(props)
        return Hydrate(instance) { ... }
    end
end

Component(gui.Thing.OtherThing) {
    Value = ...
}

This certainly could work, but I think we could do better to bring it closer to the kind of code we'd write with New.

@Anaminus
Copy link
Contributor Author

What if Hydrate could be instructed to operate within a tree?

  • Hydrate(inst: Instance): Receives an instance directly, returns the construction as usual.
  • Find(name: string): Returns a definition that will be resolved by Hydrate. Hydrate will operate on the first child named name.
local message = State("Hello, world")
Hydrate(PlayerGui.MyGui) {
	-- Finds PlayerGui.MyGui.Thing
	[Children] = Find "Thing" {
		[Children] = {
			-- Finds PlayerGui.MyGui.Thing.OtherThing
			Find "OtherThing" {
				Text = message,
			},
			-- May also construct new instances.
			New "TextLabel" {
				Name = "AdditionalThing",
			},
			-- May also insert more instances.
			Hydrate(PlayerGui.Template:Clone()) {
				-- etc
			}
		},
	},
}

A neat side-effect is that it enables safe tree descension for free, though whether to use FindFirstChild or WaitForChild is another question.

@dphfox
Copy link
Owner

dphfox commented Aug 30, 2021

That could certainly be done! I don't quite know how I feel about it, so I'll take some time to think about it some more. The safe tree descension is an interesting point.

@dphfox dphfox added the not ready - evaluating Currently gauging feedback label Sep 29, 2021
@dphfox dphfox added not ready - design wanted Good idea but needs design work and removed not ready - evaluating Currently gauging feedback labels Nov 3, 2021
@dphfox
Copy link
Owner

dphfox commented Dec 6, 2021

So I think I have a somewhat alright design idea for this - it'll require a bit of extension of how [Children] works but nothing we can't do.

Hydrate() would take either an Instance or a string. The Instance form immediately applies the properties and returns the instance, while the string form returns an UnresolvedChild object to be completed later. Then, we allow UnresolvedChild objects to be passed to [Children] - these objects are first converted to instances by trying to :FindFirstChild() on the parent, then are treated as instances thereafter.

Instance form

local function Hydrate(instance: Instance)
    return function(props: PropertyTable): Instance
         -- apply props to instance, just like New...
         return instance
    end
end

String form

local function Hydrate(childName: string)
    return function(props: PropertyTable): UnresolvedChild
         return {
             type = "UnresolvedChild",
             childName = childName,
             props = props
         }
    end
end

Hypothetical usage

local prefab = ReplicatedStorage:WaitForChild("UI")

Hydrate (prefab:Clone()) {
    Parent = Players.LocalPlayer:FindFirstChildWhichIsA("PlayerGui"),

    [Children] = {
        Hydrate "Button" {
                Text = "Hello, Fusion"
        },

        New "ImageLabel" {
            Name = "Icon",
            Image = "example://12345"
        }
    }
}

I like this approach because it means that Hydrate and New can be interwoven by default - you can even use unresolved Hydrate inside of a New call if you wanted to, though that'd probably be useless as new instances don't have children.

@dphfox
Copy link
Owner

dphfox commented Dec 6, 2021

As a slight alternative, we can keep these two forms as separate functions as suggested above - this is probably a more honest API design. Names tweaked for consistency without being overly verbose:

local prefab = ReplicatedStorage:WaitForChild("UI")

With (prefab:Clone()) {
    Parent = Players.LocalPlayer:FindFirstChildWhichIsA("PlayerGui"),

    [Children] = {
        WithChild "Button" {
                Text = "Hello, Fusion"
        },

        New "ImageLabel" {
            Name = "Icon",
            Image = "example://12345"
        }
    }
}

@nezuo
Copy link

nezuo commented Dec 6, 2021

I don't think there should be the string form of Hydrate. I think the traversal should be up the the user.

Example:

local prefab = ReplicatedStorage:WaitForChild("UI")
local ui = prefab:Clone()
local button = ui:FindFirstChild("Button") -- I would personally use a function that errors if the child isn't there

Hydrate (prefab:Clone()) {
	Parent = Players.LocalPlayer:FindFirstChildWhichIsA("PlayerGui"),

	[Children] = {
		Hydrate (button) {
			Text = "Hello, Fusion",
		},

		New "ImageLabel" {
			Name = "Icon",
			Image = "example://12345",
		},
	},
}

This method is still possible with the addition of the string form, but I don't think you should make assumptions about how the user wants to traverse the tree (FindFirstChild vs WaitForChild for example). It also goes against the "everything is just an instance" model.

@dphfox
Copy link
Owner

dphfox commented Dec 7, 2021

I don't think there should be the string form of Hydrate. I think the traversal should be up the the user.

Example: [snip]

This approach, while conceptually cleaner, doesn't scale nicely for deeply nested UI. See from above how New scales better than Hydrate in this respect:

-- when dealing with children, the syntax is clean and your code has strong visual hierachy
local gui = New "Thing" {
    [Children] = New "Thing" {
        [Children] = New "Thing" {...}
    }
}

-- dealing with children is much more verbose and lacks the same visual structure
local gui = PlayerGui.MyGui
Hydrate(gui) {
    Value = ...
}
Hydrate(gui.Thing) {
    Value = ...
}
Hydrate(gui.Thing.OtherThing) {
    Value = ...
}

-- you could nest them but now the parent will be set and tracked redundantly
-- this also doesn't solve the problem with indexing
Hydrate(gui) {
    [Children] = Hydrate(gui.Thing) {
        [Children] = Hydrate(gui.Thing.OtherThing) {
            Value = ...
        }
    }
}

-- this also becomes quickly horrendous with FFC
Hydrate(gui) {
    [Children] = Hydrate(gui:FindFirstChild("Thing")) {
        [Children] = Hydrate(gui.:FindFirstChild("Thing"):FindFirstChild("OtherThing")) {
            Value = ...
        }
    }
}

-- compared to unresolved children, which are much more in line with New
Hydrate(gui) {
    [Children] = Hydrate "Thing" {
        [Children] = Hydrate "OtherThing" {
            Value = ...
        }
    }
}

This method is still possible with the addition of the string form, but I don't think you should make assumptions about how the user wants to traverse the tree (FindFirstChild vs WaitForChild for example). It also goes against the "everything is just an instance" model.

This is a somewhat valid point, but also everything isn't an instance already. Children can also be state objects or arrays.

@Gargafield
Copy link
Contributor

Why not allow new to take in a string or an instance? Seems a lot easier, although the naming is a bit weird.

@dphfox
Copy link
Owner

dphfox commented Dec 7, 2021

Why not allow new to take in a string or an instance? Seems a lot easier, although the naming is a bit weird.

  1. New already takes in strings
  2. New is functionally different from Hydrate, since New constructs a new instance while Hydrate uses an existing instance

@Anaminus
Copy link
Contributor Author

Anaminus commented Dec 7, 2021

When we say something like WithChild "Thing", what do we mean? Is it the "Thing" at the current time, or whatever "Thing" will be at any point in time? The latter is more complicated, but also more Fusiony. Now it's no longer about FindFirstChild, but incorporating the names of children into the dependency graph. After that, we're suddenly thinking about incorporating any arbitrary criteria. Sounds a lot like CSS.

How this might work:

  • A means to build a "selector". Might constructed out of symbols somehow, or could be parsed from a string (I hear you already have something like this).
  • Symbols that evaluate a selector, then apply properties to anything that matches. Has two flavors:
    • A "query" symbol that evaluates a selector once. Less expensive. Mainly used to target specific objects. This implements what we've been considering so far.
    • A "rule" symbol that creates a living selector. More expensive. Targets anything matching the selector at any time. Implements what I described above.
  • Existing symbols like Children could be used to limit the scope.

The problem of getting an existing instance to bind to could be reinterpreted as a selector where the operand is an actual instance that must be matched. Or a selector that is an instance just selects that instance.

In any case, I'm not sure whether or not this deviates from the original problem enough to require its own issue.

@Gargafield
Copy link
Contributor

Why not allow new to take in a string or an instance? Seems a lot easier, although the naming is a bit weird.

  1. New already takes in strings
  2. New is functionally different from Hydrate, since New constructs a new instance while Hydrate uses an existing instance

I think you misunderstood. I think that New should create an instance if a string is passed, and if an instance is passed it should just use that. New still binds stuff, but you now have two options for New.
New should be renamed if this was the solution.

Heres an example:

-- Valid
New("Frame")({
    Parent = PlayerGui
    -- Change things
})

-- Also valid
New(PlayerGui.Frame)({
    -- Change things
})

@dphfox
Copy link
Owner

dphfox commented Dec 8, 2021

I think you misunderstood. I think that New should create an instance if a string is passed, and if an instance is passed it should just use that. New still binds stuff, but you now have two options for New.
New should be renamed if this was the solution.

I'm not really convinced this is the right approach - New is a special case of Hydrate, not the other way around, so New shouldn't be overloaded.

It'd instead make more sense to overload Hydrate - the first example is what New already does functionally:

Hydrate (Instance.new("Frame")) {
    Parent = PlayerGui,
    -- and more props...
}

Hydrate (PlayerGui.Frame) {
    -- more props...
}

At this point, New would then just become a syntax sugar around Hydrate:

local function New(className: string)
    return Hydrate(Instance.new(className))
end

New "Frame" {
    Parent = PlayerGui,
    -- and more props...
}

-- is equivalent to
Hydrate (Instance.new("Frame")) {
    Parent = PlayerGui,
    -- and more props...
}

Something worth noting here - if this is the direction we go in, then this becomes another case against implementing #46 as we'd now be taking advantage of partial application at the library level.

@nezuo
Copy link

nezuo commented Dec 8, 2021

At this point, New would then just become a syntax sugar around Hydrate:

I think New would have to be like this since you don't want to override the properties of instances you are hydrating.

local function New(className)
	local instance = Instance.new(className)

	applyDefaultProperties(instance, className)

	return Hydrate(instance)
end

You can still take advantage of partial application with the Roact-like API:

local function New(className, propertyTable): Instance
	local instance = Instance.new(className)

	applyDefaultProperties(instance, className)

	return Hydrate(instance, propertyTable)
end

What would happen in this case?

New "TextLabel" {
	[Children] = {
		WithChild "Child" {}
	}
}

@dphfox
Copy link
Owner

dphfox commented Dec 9, 2021

I think New would have to be like this since you don't want to override the properties of instances you are hydrating.

Thanks for pointing this out! I can see that I didn't really think through my earlier post here very well, and really appreciate the corrections 🙂

You can still take advantage of partial application with the Roact-like API:

This is not partial application. Partial application (which comes from lambda calculus iirc) is when you 'fix' some of the arguments to a function ahead of time, to create a new function which implies those fixed arguments for you.

For example:

local function add(a)
    return function(b)
        return a + b
    end
end

-- you can add two numbers together
print(add(2)(2)) -- 4

-- or you can create a new function by fixing the first argument:
local increment = add(1)

print(increment(15)) -- 16

What would happen in this case?

This:

New "TextLabel" {
	[Children] = {
		WithChild "Child" {}
	}
}

is (for the purposes we care about in this example) equivalent to:

local label = Instance.new("TextLabel")
-- apply default props, etc.
Hydrate(label) {
	[Children] = {
		WithChild "Child" {}
	}
}

When Hydrate attempts to resolve the UnresolvedChild returned by WithChild "Child" {}, it won't be able to find any instance with that name, because freshly made instances don't have children in them. I suspect we will end up handling this case by throwing an error, unless there's a case to be made for some other behaviour.

@nezuo
Copy link

nezuo commented Dec 9, 2021

This is not partial application.

Yeah that's my bad, I looked up a definition from an article and it just said "reusing common parts." So I thought reusing the Hydrate function was partial application.

I suspect we will end up handling this case by throwing an error

One thing I want to note is that if you removed WithChild and only let people hydrate instances they have access to, it would be impossible to represent this in code.

@dphfox
Copy link
Owner

dphfox commented Dec 10, 2021

One thing I want to note is that if you removed WithChild and only let people hydrate instances they have access to, it would be impossible to represent this in code.

Yeah - I'm not entirely sold on it being a good idea. I'm much more confident in Hydrate, so we could probably split this into two halves and ship Hydrate first, while we figure out whether WithChild or some equivalent is a good idea to go ahead with.

@dphfox
Copy link
Owner

dphfox commented Feb 1, 2022

I'll be closing this issue for now, since the main feature request (Hydrate) has now been completed. I'm moving discussion of WithChild and related ideas into #124 👍🏻

@dphfox dphfox closed this as completed Feb 1, 2022
@dphfox dphfox linked a pull request Feb 3, 2022 that will close this issue
@dphfox dphfox added ready to work on Enhancements/changes ready to be made and removed not ready - design wanted Good idea but needs design work labels Feb 3, 2022
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

Successfully merging a pull request may close this issue.

4 participants