Fusion icon indicating copy to clipboard operation
Fusion copied to clipboard

Bulk apply properties/events/tags/attributes

Open dphfox opened this issue 3 years ago • 11 comments

Currently in Fusion, when dealing with common properties like Position or Size on components, you end up having to pass through lots of those properties very often:

local function Button(props)
	local isHovering = Value(false)
	local isPressed = Value(false)

	return New "TextButton" {
		Name = "Button",

		Position = props.Position,
		AnchorPoint = props.AnchorPoint,
		LayoutOrder = props.LayoutOrder,
		Visible = props.Visible,
		ZIndex = props.ZIndex,

		...

This arises from the generally-good encapsulation which components enforce - it explicitly defines which properties this component should accept, and how those properties will be used, without the user of the component having to worry about this.

One possible solution to this issue is the user using Hydrate - however this immediately kills off encapsulation and starts making assumptions about the internal workings of components, plus it works less acceptably for arrays or children or state containing children.

Therefore, it's worth considering whether we should introduce some better solution built into Fusion for this kind of property passthrough.

dphfox avatar Feb 18 '22 18:02 dphfox

One initial vague thought I had was to use a special key generator, kind of like OnEvent, to add a syntax sugar for property passthrough from a property table:

local function Button(props)
	local isHovering = Value(false)
	local isPressed = Value(false)

	return New "TextButton" {
		Name = "Button",

		[Passthrough(props)] = {"Position", "AnchorPoint", "LayoutOrder", "Visible", "ZIndex"}

		...

dphfox avatar Feb 18 '22 18:02 dphfox

A more general solution, which has previously been mentioned in other contexts such as implementing design tokens, would be a Merge or Apply key which allows for the merging of property tables (and perhaps greater interoperability with libraries like Llama for table manipulation):

local function subset(tbl, keys)
    local new = {}
    for key, value in pairs(tbl) do
        if table.find(keys, key) ~= nil then
            tbl[key] = value
        end
    end
    return new
end
end

local function Button(props)
	local isHovering = Value(false)
	local isPressed = Value(false)

	return New "TextButton" {
		Name = "Button",

		[Apply] = {subset(props, {"Position", "AnchorPoint", "LayoutOrder", "Visible", "ZIndex"})}

		...

dphfox avatar Feb 18 '22 18:02 dphfox

Of course, there's also the current pre-existing solution, which is to operate on the property table directly, though this is a bit messy:

local function Button(props)
	local isHovering = Value(false)
	local isPressed = Value(false)

	return New "TextButton" (merge(props, {
		Name = "Button",
		...

dphfox avatar Feb 18 '22 18:02 dphfox

It's worth understanding why this isn't a problem for non-Roblox UI frameworks, too. In the context of web development, this tends not to be an issue, because things like layout and styling are not intrinsic to component HTML. Instead, styling and layout is handled by CSS and the browser's layout engine.

Roblox does not have this, instead opting to tie style directly to instances. Perhaps a more broadly useful solution here would be to start thinking about third party libraries for better layout and styling?

dphfox avatar Feb 18 '22 18:02 dphfox

In TypeScript, this is very easy, using the following:

const mergedProps = {
	Name: "Button",
	...props,
}

This will shallow-copy all keys of props, except for those specified in the literal. Unfortunately, Lua has no direct support for this behaviour, but I felt it was valuable to point out for consideration.

Dionysusnu avatar Feb 18 '22 22:02 Dionysusnu

I do think it's important to consider that simply allowing arbitrary props to be set would likely be a bad idea for encapsulation purposes (and can be done already via Hydrate anyway). I think it is important to allow restricting property passthrough to certain props like Position and AnchorPoint in order to maintain a clean explicit divide between interface and implementation.

dphfox avatar Feb 19 '22 04:02 dphfox

We also need to think about special keys like event and out.

lukadev-0 avatar Dec 25 '22 20:12 lukadev-0

Related to #206

dphfox avatar Feb 01 '23 01:02 dphfox

Related to #112

dphfox avatar Aug 23 '23 11:08 dphfox

I'm broadening the scope of this issue to include bulk setting as a general concept for instances, not just limited to properties, as I explain in #36.

The questions now:

  • Is there a nice symmetric API design that neatly allows for bulk-setting properties, that we can also re-use for bulk-setting attributes, events, and CollectionService tags?
  • Does it make sense to include all of these as bulk operation types?

dphfox avatar Feb 05 '24 15:02 dphfox

Here's my idea for how this could work. I'll use a hypothetical [Properties] tag as my example.

Obviously, a bulk-setting [Properties] tag should accept a bunch of properties to set at once:

[Properties] = {
    Position = props.Layout.Position,
    AnchorPoint = props.Layout.AnchorPoint,
    Size = props.Layout.Size,

    BackgroundColor3 = Color3.new(0.5, 0.7, 1),
    TextColor3 = Color3.new(1, 1, 1)
}

However, this doesn't solve for cases where we want to merge multiple property tables together. So, what if you could optionally pass in multiple property tables, and have them be merged together?

[Properties] = {
    {
        Position = props.Layout.Position,
        AnchorPoint = props.Layout.AnchorPoint,
        Size = props.Layout.Size,
    },
    {
        BackgroundColor3 = Color3.new(0.5, 0.7, 1),
        TextColor3 = Color3.new(1, 1, 1)
    }
}

This is an ergonomic thing that people have wanted to do for a while now (and is something we're breaking by removing Hydrate) so this seems like it could be a good way to account for that.

The above code snippet could even be reduced further:

[Properties] = {
    props.Layout,
    {
        BackgroundColor3 = Color3.new(0.5, 0.7, 1),
        TextColor3 = Color3.new(1, 1, 1)
    }
}

This syntax can be easily aligned with the syntax for many other parts of Fusion dealing with these sorts of 'optionally array' parameters. Specifically, [Properties] could mirror [Children] by defining itself recursively:

export type OneOrMore<T> = T | {OneOrMore<T>}

export type Children = OneOrMore<CanBeState<Instance>>
export type Properties = OneOrMore<PropertyTable>

This works so long as OneOrMore<T> does not receive an array type for T.

Obvious questions arise around what happens when a property is defined in multiple property tables. When Fusion encounters conflicts like these, it traditionally attempts to throw an error, as Fusion's philosophy is that things should be defined in one consistent place. By assuming that multiple definitions are a declaration of intent for one definition to be chosen silently, it suppresses mistakes made when multiple definitions are passed by accident.

That being said, it's not entirely out of the question that we could, at some stage, introduce opt-in overwriting features that let you specify 'default' values when you intend for something to only be used in lieu of other definitions.

This could be done by annotating fields with a kind of Default key:

[Properties] = {
    props.Layout,
    {
        -- only Default values can be overridden
        [Default "Position"] = UDim2.fromScale(0.5, 0.5),
        [Default "Size"] = UDim2.fromScale(1, 1),
        [Default "AnchorPoint"] = Vector2.new(0.5, 0.5),

        BackgroundColor3 = Color3.new(0.5, 0.7, 1),
        TextColor3 = Color3.new(1, 1, 1)
    }
}

However, it's conceivable that multiple "defaults" might be specified, especially in systems where these sorts of templates are expected to be composed atop each other, raising questions about which default takes priority:

local TEXT_STYLE = {
    [Default "TextSize"] = 14,
    [Default "Font"] = Enum.Font.SourceSans
}

local HEADER_TEXT_STYLE = {
    [Default "TextSize"] = 28
}

-- Which default TextSize should be used?
[Properties] = {TEXT_STYLE, HEADER_TEXT_STYLE}

An arbitrary rule could be adopted in theory where order is significant; perhaps earlier property tables are conceptually applied first, while later property tables overwrite them. Alternatively, multiple defaults could be disallowed entirely, and developers could be explicitly encouraged to break up these templates further to avoid the conflict. A priority number could be included to tiebreak defaults in an order-independent manner.

I think the complexity of these considerations leads me to the conclusion that Fusion should take the most conservative stance on duplication and overwriting at this early stage. I propose that duplicates of any kind should be disallowed, and that defaults and overwriting behaviour are excluded from this feature for the foreseeable future, until we can collectively decide on a direction to take this at a later date.

What I do think we can conclude though, is that there is indeed a nice formulation of this bulk-setting for merging together property tables defining unique keys, and that this general pattern should be extensible to bulk-setting events, property change events, attributes, tags, and anything else we would like (so long as the T for its OneOrMore<T> type is not an array).

dphfox avatar Feb 22 '24 16:02 dphfox