Add destructors to `Computed`

[dphfox]

As we plan to move Fusion beyond depending on garbage collection for UI, we need to address more structural problems in Fusion which we currently rely on garbage collection to solve. One of those things is cleaning up values returned by computeds.

I've identified two common cases we need to solve for. Firstly, we might be using Computed to generate new values, for example:

local className = Value("Frame")
local foo = Computed(function(use)
    -- when we change the `className`, we'd like the old thing to get destroyed and replaced by the new thing
    return New (use(className)) {
        Name = "Thing"
    }
end)

Secondly, we might be passing through values from elsewhere, for example:

local ServerStorage  = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local isServer = Value(false)
local storageService = Computed(function(use)
    -- we *don't* want to destroy the old game service here!
    return if use(isServer) then ServerStorage else ReplicatedStorage
end)

Of course, the lifetimes of the returned values under garbage collection can be much more complex and implicit than this, but I think these are the primary uses of Computed that we should worry about.

One neat solution that we already employ with the For objects is destructor functions. Notably, these objects mostly deal with generating new values, but it does still support passing through values if you replace the destructor function with an empty function:

local services = ForValues(
    {"Workspace", "Lighting", "ServerStorage"},
    function(name)
        return game:GetService(name)
    end,
    function(service)
        -- intentionally don't do anything in the destructor - we don't need to destroy anything here
    end
)

I propose that we should allow for Computed objects to use these destructor functions as well. For example:

local className = Value("Frame")
local foo = Computed(function(use)
    return New (use(className)) {
        Name = "Thing"
    }
end, function(instance)
    instance:Destroy() -- destroy the instance explicitly when we're done with it
end)

local ServerStorage  = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local isServer = Value(false)
local storageService = Computed(function(use)
    return if use(isServer) then ServerStorage else ReplicatedStorage
end)
-- no extra destructor necessary - we don't need to destroy anything here

Optionally, we could also provide the cleanup function as the default destructor, which is used if the user does not provide one explicitly (similar to how the For objects work). There are some questions about whether this is the intuitive behaviour or not:

local className = Value("Frame")
local foo = Computed(function(use)
    return New (use(className)) {
        Name = "Thing"
    }
end)
-- Fusion's cleanup() will automatically destroy these instances for us

local ServerStorage  = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local isServer = Value(false)
local storageService = Computed(function(use)
    return if use(isServer) then ServerStorage else ReplicatedStorage
end,  function(service)
    -- intentionally don't do anything in the destructor - we don't need to destroy anything here
end)

Alternatively, we could simply expose cleanup as a member of the Fusion API rather than making it the default. This is probably the most convenient explicit option:

local className = Value("Frame")
local foo = Computed(function(use)
    return New (use(className)) {
        Name = "Thing"
    }
end, Fusion.cleanup) -- same as above!

local ServerStorage  = game:GetService("ServerStorage")
local ReplicatedStorage = game:GetService("ReplicatedStorage")

local isServer = Value(false)
local storageService = Computed(function(use)
    return if use(isServer) then ServerStorage else ReplicatedStorage
end)

Of course, we can also ask further questions about what objects the cleanup function should cover. Right now, it:

• calls :Destroy() on values of type Instance
• calls :Disconnect() on values of type RBXScriptConnection
• calls :Destroy() on values of type {Destroy: () -> ()}
• calls :destroy() on values of type {destroy: () -> ()}
• iterates using ipairs over values of type {[1] = any} - calls cleanup() on each sub-element

This largely matches the existing behaviour of Maids, with the exception of the last addition which supports cleaning up nested arrays of objects. Should we add to this list, remove from it or keep it the same? (this is probably a good topic for another issue if needed)

Whatever option we choose for the default here though, we should also make the For objects match this behaviour for internal consistency.

[Zyrakia]

Would love this, been having problems of cleanup not happening as soon as it could be, leaving a lot of heavy UI in memory, and this would intuitively solve it.

Although the default being the same as For* scares me since I don't know how many computed I have that pull an instance out of an array based on an index dependency. It'll definitely be a chore double checking every computed since it's probably the most used Fusion object within a lot of projects, but for consistency, I do agree, it may be worth it.

Out of curiosity, has the cleanup function been benchmarked? I don't think it will create much a performance impact but it'd be good to know.

[Zyrakia]

I was wondering if it may be feasible to add a new object info Fusion that fills this need. The, however small, overhead of the cleanup function is not necessary for most computeds, and omitting it will only result in a more cluttered computed statement.

I feel that if most use cases of computeds have no need for a cleanup function, but some uses have an urgent need for it, the cleanest solution may be to break these two types of computeds into separate state objects.

I am unsure about naming in this case because I feel like if not given a lot of care it can cause a lot of confusion.

[dphfox]

I don't think introducing a new object is appropriate here. Conceptually, the lifetime of a value should be intrinsic to the value itself - state objects are merely vehicles for moving these values around. Therefore, there should be no difference between a Computed handling things that need cleanup versus a Computed handling things that may be quietly disposed.

I would be wary of talking about overhead without substantial performance metrics to back this up. While it's true that it's not maximally efficient to call a function on all values passing through a computed, the most that would happen for an incompatible type of value would be an if-statement not matching, which is overhead on the scale of microseconds and of no interest in the performance equation. The real source of performance issues is much more likely to be in whatever the user is doing, not Fusion itself, in this case.

I think that the best way to go about this is explicitly adding the cleanup function as a destructor, as in example 3, as this makes it the most clear that cleanup behaviour will occur in it's presence, or will not occur in it's absence.