dphfox / Fusion

A modern reactive UI library, built specifically for Roblox and Luau.
https://elttob.uk/Fusion/
MIT License
596 stars 103 forks source link

Bulk apply properties/events/tags/attributes #137

Open dphfox opened 2 years ago

dphfox commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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?

Dionysusnu commented 2 years ago

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.

dphfox commented 2 years ago

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.

lukadev-0 commented 1 year ago

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

dphfox commented 1 year ago

Related to #206

dphfox commented 1 year ago

Related to #112

dphfox commented 9 months ago

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:

dphfox commented 8 months ago

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).