dphfox / Fusion

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

Catching double child assignment #108

Open dphfox opened 2 years ago

dphfox commented 2 years ago

Currently in Fusion, the following code will not error, and place both instances in folder1. This is because [Children] doesn't override a manual Parent definition:


local folder1 = New "Folder" {}

local instance1 = New "Part" {
    Parent = folder1
}

local instance2 = New "Part" {
    Parent = Value(folder1)
}

local folder2 = New "Folder" {
    [Children] = {instance1, instance2}
}

The following code also doesn't throw an error, but the parent of instance1 is undefined and prone to switching dependent on the internal implementation of Fusion:

local instance1 = New "Part" {}

local folder1 = New "Folder" {
    [Children] = {instance1}
}

local folder2 = New "Folder" {
    [Children] = {instance1}
}

I think that in any case where an instance's ancestry is defined twice, we should throw an error, including cases where we currently allow it by giving Parent priority. This is a breaking change but it's likely a really bad idea to introduce this kind of ambiguity into a codebase anyway when everything is meant to be declarative, final and guaranteed to hold at all times.

nottirb commented 2 years ago

it's likely a really bad idea to introduce this kind of ambiguity into a codebase anyway when everything is meant to be declarative, final and guaranteed to hold at all times

If you're defining an instance's ancestry twice, it can lead to some interesting issues, and I can't think of any good use cases for doing that anyways.

Additionally, if you're re-using some part of your UI you typically should be using components. If anything, erroring on the second example forces people to think about how they're setting up their UI.

For that reason I think this would be a good addition.

dphfox commented 1 year ago

Waiting for #206 to see how we should implement this.

dphfox commented 2 months ago

Unblocking this. We won't need double parent assignment - that should be impossible now - but it's still possible to attempt to parent one child into two places. I'm not sure how urgent this is though, so I'm probably not going to tackle it until down the road.