Should peek() deep copy by default?

[dphfox]

This is proposed as an alternate solution to the problems set out in #44.

It may be more straightforward to make :get() (on all state objects) perform a deep copy if the state object is holding a table value.

This would achieve the same results that force = true achieves now, except without the possibility of mutating the state object's current value.

For cases where the exact reference is important, we could also introduce an option to return the raw (uncloned) value.

Considerations:

• Deep equality checking #44 may be lighter for more complex graphs as work is done only when table values are generated by state objects - the performance of :get() is unchanged. Here, we would be doing work in every single :get() call that returns a table
• Deep equality checking #44 is less 'safe' in that it encourages state mutation - here, code would be 'purer' and more immutable, though it remains to be seen whether the theoretical benefits of this materialise in widespread usage
• Deep equality checking #44 requires modification of the :update() interface for dependent graph objects, while here we're modifying the :get() interface for state objects. Essentially we're trading off whether the responsibility of handling table state lies with the provider of the state (don't send identical state to dependents by making all user-retrieved state non-identical) or with the receiver of the state (don't update when dependency's state is identical as determined by deep equality checking)

I think that in order to make the right call here we just have to test these two approaches out with real code - both seem like valid approaches for different and often incomparable reasons. It's worth noting however that this approach is not a complete solution to the problem, but rather a 'good-enough' approach that allows users to get the results they probably wanted.

[dphfox]

One other consequence of this approach is that we would also retain the same redundancy problems we currently have with force = true - namely that operations like state:set(state:get()) would now cause updates that are almost certainly unnecessary.

[dphfox]

An interesting gotcha here - while reference-type tables are not useful for dealing with most things, they are useful when dealing with some things like objects where reference-style equality is useful.

Consider, for example, storing a Value inside a Value; in this case, it makes sense that you get back the same Value you put in:

local subValue = Value()
local superValue = Value(subValue)

print(superValue:get() == subValue) -- true - makes sense!

This is a useful case to support, because you can use it to dynamically switch out dependencies and values:

-- healths are Values because they can change at any time
local player1Health = Value(100)
local player2Health = Value(50)
local player3Health = Value(50)

-- we want to select one player's health to show
local selectedHealth = Value(player1Health)

-- process the selected player's health into a message
local message = Computed(function()
    local healthObject = selectedHealth:get()
    return "This player has " .. healthObject:get() .. " HP."
end)

This is a specific example of a general truth - if we are going to implement some kind of deep clone, we will need to support both objects and tables in the same data structures.

Consider this data structure - if we were to deep copy this, we want to keep the objects intact, but the tables containing them should be cloned:

local myData = {
    player1 = {
        health = Value(100),
        armour = {head = Value(100), torso = Value(100), legs = Value(100), feet = Value(100)}
    },
    player2 = {
        health = Value(100),
        armour = {head = Value(100), torso = Value(100), legs = Value(100), feet = Value(100)}
    }
}

Something that this has led me to realise is that 'share-ability' is a property of tables - we will need some way of marking a table as 'cloneable' or 'shareable'. I propose that we should make 'cloneable' the default so that tables have value-like semantics by default and have the most intuitive behaviour, with the more performant 'shareable' behaviour being opt-in. This rules out using table.isfrozen for the purpose of indicating clonability.

So far, I see a few ways of marking a table as 'shareable':

• Detecting the presence of a type string, i.e. type = "State"
  • Fusion objects get support automatically!
  • Very third-party compatible
  • I really hope your data doesn't contain a type field storing a string by pure coincidence
  • Very easy developer experience
• Using a special key, i.e. [Fusion.shareable] = true
  • This will likely not work well if the same table is used across multiple installs of Fusion
  • Kind of third-party compatible
  • This would be ridiculously hard to accidentally add, unlike the type string option
  • Relatively easy developer experience
• Using a custom metatable field, i.e. getmetatable().shareable = true
  • Does not depend on any special values specific to Fusion installs
  • Very third-party compatible
  • Still pretty hard to accidentally add, unless you like abusing metatables
  • Not quite as good developer experience

In my mind, the metatable option makes most sense - after all, we want to add metadata to a table, not data. Metatables are useful because they describe a table without requiring modifying their contents. It seems like the most logical place to put a field which influences the behaviour of the table, even if it's a custom field. (if we do this though, we definitely should not use double underscores, since this is meant to be reserved for Luau fields)

To help out the DX, we could expose a utility function for making a table shareable:

local function shareable(t)
    getmetatable(t).shareable = true
    return t
end

local willBeCopied = {1, 2, 3, 4, 5}
local willBeShared = shareable {1, 2, 3, 4, 5}

I'd definitely like further thoughts on this. I have approximately zero idea what I'm doing here lol

[dphfox]

To be clear - the goal here is making table data act more like a value-type, while not breaking objects.

[dphfox]

I think that this is a good idea, but we'll need to test it out in the real world to evaluate things like the performance impact of doing this for most tables by default.

[jmkd3v]

Here's a crazy and bad idea, but maybe worth thinking about: :get() could do something sneaky when returning a table, like proxying it and watching for __newindex to mark it as "edited", or maybe the value object could contain its own shallow copy of the table at all times (updated upon :set()) which it compares with a shallow compare?

[dphfox]

I see you are a Swift user ;)

Jokes aside, copy-on-edit is certainly something you could do, but I don't think that's a very elegant solution to the problem. I think we could maybe get away with a shallow copy.

[Hexcede]

@Elttob I have a fairly big proposal here to solve this issue, I tried to remain mostly faithful to existing Fusion features, and still supporting all use cases (but without too much hassle).

I'd actually somewhat prefer the current behaviour, despite the footgun in the example. The reason being that it supports all use cases out of the box, with minimal changes. From the referenced issue #44, if I want to implement the copy behaviour as an end user it's easy to do:

local array = {1, 2, 3, 4, 5}
local state = State(array)

table.insert(array, 6)
state:set(table.clone(array))

However, I believe mutations sort of go against the nature of Fusion anyways. Perhaps it would be better to provide the user with a way to mutate data in a Fusion-managed form, with the behaviour being an extra.

---

Proposal

I would propose a state wrapper similar to Tween, Spring, etc which is used for states that control mutable data:

local array = {1, 2, 3, 4, 5}
local immutableState = Value(array) -- Naming aside, immutableState's value is still mutable (currently*)

local mutableState = Mutable(immutableState) -- Wraps immutableState, which maintains a mutable copy of the underlying data.
local mutableArray = mutableState:get() -- We grab our table
table.insert(mutableArray, 6) -- Make some changes
mutableState:set(mutableArray) -- Update the mutable version of the array

Behaviour

• mutableState:set() should:

  • Update immutableState if a different table reference (or type) is passed
    • immutableState:get() and its underlying value should be treated as the same reference
  • Generate a diff (shallow or deep) if the reference is identical, which can be applied to immutableState's underlying value.
    • If the diff is empty (no changes), it should do nothing, there are no changes.

• mutableState:get() should be:

  • Be the same reference on every call, except when immutableState's reference has changed.
  • Be synchronized with immutableState. When immutableState changes, we know to sync mutableState's array.

• Furthermore, this provides Fusion the option to handle metatable mutations in the same way. (This likely cannot work correctly with __metatable, so there is that to consider)

• • This would allow for Value:get() to return frozen copies (shallow or deep) except with a flag, similar to some of the original ideas for this issue.
    • For compatibility, users could easily wrap the value in a Mutable wrapper, which can directly make changes to the underlying value, similar to the existing behaviour
    • This would ultimately discourage mutating data, and when mutating data is involved, Fusion can make any number of decisions about those mutations
    • immutableState:set() should:
      • Treat its :get() reference equally to the underlying value's reference (doesn't trigger state changes)
      • Update its :get() reference when the data changes (when the state changes)

This maintains the ability for the user to do arbitrary mutations, but enforces Fusion's control over said mutations. This means the user code stays happy, and it solves the original footgun in a way that's consistent with the existing Fusion patterns.

Final justifications

I believe the ability to perform mutations is useful, even if it is a bad pattern. Similar to Hydrate, it allows bridging "bad" (for lack of a better word) code with "good" Fusion code that follows best practices by offering a compromise between the two.

In contrast to some other solutions, old code can be easily adapted like so and remain fully compatible:

-- Old
local myTable = Value({1, 2, 3})
myTable:get()[3] = 4
myTable:set(myTable:get(), true)

-- New
local myTable = Mutable(Value({1, 2, 3}))
myTable:get()[3] = 4
myTable:set(myTable:get())

The only exceptions here are that myTable:get() will be a different reference, so if references are needed, Mutable just doesn't get wrapped, and forced :set() gets used again.

[dphfox]

@Hexcede you make some valuable points about preserving existing use cases here. We should definitely look into some way of ensuring both immutable and mutable use cases are supported pretty optimally. Thanks for the detailed post!

Using an intermediate object is an interesting approach but the problem is that there is no standardised API for setting values on state objects, because different state objects derive their values from different sources and methodologies by design. I think perhaps it'd be ultimately simpler to implement and simpler to extend if state objects provided their own routines for handling both paths.

We could possibly look at splitting Value's set method into two separate methods that each optimise for immutable and mutable sets respectively. We would need to name them according to verbs that intuitively represent the kind of use cases they should be used for so that newer users can grasp the concept. I think that this is probably a good approach, though I'll need a little more time to become confident in that conviction.

[krypt102]

This might be redundant due to #216 being written.

[dphfox]

Here's an interesting proposal; in the spirit of #291, we could conceivably use a table's frozen status to decide whether it should be cloned or not.

[dphfox]

Here's an interesting proposal; in the spirit of #291, we could conceivably use a table's frozen status to decide whether it should be cloned or not.

This is now implemented, by the way.

[dphfox]

We will probably not do this. Changing the behaviour of peek in this way would likely be more confusing, and we have better handling of mutation these days.