Should Fusion introduce a Roact-like form of the New function?

[dphfox]

It was suggested in a recent PR that we could consider adding a more Roact-like form of the New function, accepting all arguments in one call rather than as a curried function. While I rejected the PR on the grounds that changes should be discussed before they're added, I'm opening an issue to discuss it here :)

Hypothetical usage:

-- option 1 for children - just like we do now:
local ui = New("Frame", {
    Name = "Foo",
    Visible = true,

    [Children] = New("UICorner", {...})
})

-- option 2 for children - separate it like Roact
local ui = New("Frame", {
    Name = "Foo",
    Visible = true
}, {
    New("UICorner", {...})
})

[dphfox]

For my personal take on this - I'm personally more of the opinion that we shouldn't introduce multiple ways of doing the same thing like this. It tends to promote inconsistency across codebases, and is something that can be achieved by a ~3 line utility function implemented in user space anyway.

As for which form I'd choose if we did implement this, I'd prefer the [Children] form as opposed to the more Roact-like form, since it reads more explicitly.

[Dionysusnu]

I'm conflicted. On one hand, as you say it promotes inconsistency between codebases. On the other hand, there's not much harm in adding this, and it would, for example, greatly simplify the Roblox-ts fusion jsx differences. Currently we have to use a shim of createElement.
If we do go this route, I think both children options should be allowed. If 3 arguments, the last one is the children table. If 2, the props symbol is used.

Roblox-ts jsx has turned out different. I now vote for option 1, or not adding this at all.

[dphfox]

If we do go this route, I think both children options should be allowed. If 3 arguments, the last one is the children table. If 2, the props symbol is used.

I'm not really convinced. I think the [Children] way is more readable and is consistent with the way other special functions of New are invoked, e.g. [OnChange], [Ref], [Cleanup].

I haven't really heard any reason why three arguments would make for better developer ergonomics or tidier code, only that it'd make Fusion's API surface more Roact like. Not to say that's a bad thing (it would indeed help a few niche use cases, like JSX), but I don't think the pursuit of another framework's style should be the sole motivator for a change.

[dphfox]

With this being said, I do acknowledge there are tangible problems with the curried form of New as it stands, for example with automatic formatters, so I'd be willing to trial-run a non-curried New function for at least one beta version to gather feedback.

[Ductayp]

I vote Option 1. Much nicer on the eyes. If we wanted to be just like Roact then why not use Roact?

[dphfox]

I've been letting this stew in my mind for a while now, and I'm not convinced this is a change worth making anymore, especially in the context of newer features coming along such as instance hydration (#34).

I haven't seen much talk about this feature request for a long time, and I suspect this is something that only a small minority of Fusion users care much about. It seems like it would suffice to simply stop using Lua's syntax sugar for single-argument function calls rather than revising the API surface of New at this point.

I'm open to any further feedback on this issue for a little while longer but unless there's a good counterpoint that comes up I intend to close this feature request soon.

[nezuo]

I think 1 is the best option. I personally don't like the use of the "no parentheses function call" syntax sugar, but style is always subjective. It still has issues outside of that though. I've seen quite a few questions about the "magical" syntax, and although it's not magic, it's something a lot of Lua programmers are not aware of.

I think in the case of Hydrate the curried function call is a little strange.

local hydrateButton = Hydrate(Button)

hydrateButton({})
hydrateButton({})

With currying, you can reuse the first function call, and you really shouldn't be hydrating an instance twice. Of course this argument still falls a little flat since the alternative would be:

Hydrate(Button)({})
Hydrate(Button)({})

Either way, when I think of currying I think of reuse, so I still think the argument holds a little bit.

With the new New function proposed in #34:

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

It would also make the reusing the result of the new function not work anymore:

local newTextLabel= New("TextLabel")

-- these hydrate the same instance, instead of two different text labels:
newTextLabel({})
newTextLabel({})

[dphfox]

Hydrate is mainly curried for consistency with how New works in existing Fusion codebases, and with how unresolved children may work (if we implement them).

In the 'generation' mental model (where all UI is created from scratch by code), the classic interpretation of New is to think of it as a component generator - you first create a component, then immediately call it to create an instance:

local NewTextLabel = New "TextLabel"

-- notice the parallels with user-defined components
local myTextLabel = NewTextLabel {
    Text = "Hello, world"
}

-- all together
local myTextLabel = New "TextLabel" {
    Text = "Hello, world"
}

This is why New has historically been curried like this, and I think it makes sense to keep it that way - within the generation mental model it's a very clean definition. Not to mention it also neatly lines up with utility methods like RbxUtility.Create which some developers may be familiar with from more general Roblox development history.

It's definitely true that currying makes less sense in the new hydration mental model that we're introducing, but it'd be rather strange to have an inconsistent syntax for working with instances:

-- New inline
local A = New "TextLabel" {
    Text = "Hello, world"
}


-- New partially applied
local TextLabel = New "TextLabel"

local B = TextLabel {
    Text = "Hello, world"
}

-- Hydrate instance
local C = Hydrate(PlayerGui.TextLabel, {
    Text = "Hello, world"
})

-- Hydrate unresolved child
local D = Hydrate("TextLabel", {
    Text = "Hello, world"
})

We could either make everything look more like New, which is non-breaking and I think is better for readability and writability - typos are hard to make and obvious to spot here - and also allows for the partial application use case for those who prefer it:

-- New inline
local A = New "TextLabel" {
    Text = "Hello, world"
}

-- New partially applied
local TextLabel = New "TextLabel"

local B = TextLabel {
    Text = "Hello, world"
}

-- Hydrate instance
local C = Hydrate (PlayerGui.TextLabel) {
    Text = "Hello, world"
}

-- Hydrate unresolved child
local D = Hydrate "TextLabel" {
    Text = "Hello, world"
}

Or, we can make everything look more like Hydrate, which is a breaking change - especially for anyone using partial application - though perhaps it reflects the underlying internal implementation better:

-- New inline
local A = New("TextLabel", {
    Text = "Hello, world"
})

-- New partially applied (no longer supported inbox)
local function TextLabel(props: PropertyTable): TextLabel
    return New("TextLabel", props) :: TextLabel
end

local B = TextLabel({
    Text = "Hello, world"
})

-- Hydrate instance
local C = Hydrate(PlayerGui.TextLabel, {
    Text = "Hello, world"
})

-- Hydrate unresolved child
local D = Hydrate("TextLabel", {
    Text = "Hello, world"
})

If the primary objective is better understanding of what the code is doing, just adding parentheses already achieves that easily. Better yet, it's non-breaking and it's completely opt-in, meaning people can still use the previous syntax should they prefer it:

-- New inline
local A = New("TextLabel")({
    Text = "Hello, world"
})

-- New partially applied
local TextLabel = New("TextLabel")

local B = TextLabel({
    Text = "Hello, world"
})

-- Hydrate instance
local C = Hydrate(PlayerGui.TextLabel)({
    Text = "Hello, world"
})

-- Hydrate unresolved child
local D = Hydrate("TextLabel")({
    Text = "Hello, world"
})

I've been leaning towards this last option more and more because it solves all of our problems with educating new developers and compatibility with auto-formatters without breaking large codebases and without removing choice from the user.

Perhaps an acceptable compromise would be to simultaneously introduce an overload of New that preserves partial application behaviour, though I'm not exactly keen on it for reasons I've stated before:

-- New inline
local A = New("TextLabel", {
    Text = "Hello, world"
})

-- New partially applied
local TextLabel = New("TextLabel")

local B = TextLabel({
    Text = "Hello, world"
})

-- Hydrate instance
local C = Hydrate(PlayerGui.TextLabel, {
    Text = "Hello, world"
})

-- Hydrate unresolved child
local D = Hydrate("TextLabel", {
    Text = "Hello, world"
})

It would also make the reusing the result of the new function not work anymore:

This is a good point - thanks for bringing this up! We definitely don't want to break this behaviour.

[dphfox]

I don't think this should be a priority right now - there's currently a lot of other things with Fusion that need addressing which have very significant implications. I'm closing this issue for now, not as an indication that this is something we won't ever reconsider, but as an indication that this is not on our list of priorities right now.

Feel free to continue discussing here if you'd like :)