dphfox / Fusion

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

Explore safer interpretations of `deriveScope` #318

Closed dphfox closed 5 months ago

dphfox commented 5 months ago

Adapted from a Discord discussion:

The original proposal was to implement an Extend function for managing temporary scopes without cluttering up longer-living scopes:

local function Extend<T>(original: Fusion.Scope<T>): Fusion.Scope<T>
    local extended = Fusion.deriveScope(original)

    table.insert(original, extended)

    table.insert(extended, function()
        table.remove(original, table.find(original, extended))
    end)

    return extended
end

This is safe to do. Scopes, which are arrays of tasks, are cleaned up in reverse order of their contents. If original is being cleaned up, and has just destroyed extended, then extended is the last undestroyed value in the scope. Because there are no values to the right which matter, removing extended will not affect values that are about to be cleaned.

Dan note 1: We should probably codify that doCleanup()-ed scopes should not continue to be used in user code somehow. This was mentioned with regards to scope pooling a while back, so I'd be interested in exploring that design space further.

A secondary concern is that original might have logic for removing extended itself, and this might muck that up. However, task[index] = nil is how doCleanup() removes elements, so that's equivalent and won't be affected.

The only place this affects future cleanup values is if original contains two copies of extended, as table.find will find the copy of extended to the left.

Dan note 2: This sounds like it'll just cause a duplicate destroy, which is already what happens with duplicated scope members that don't remove themselves and is already an error.

They argue that Extend should be preferred over manual deriveScope as extend comes with numerous other benefits. deriveScope requires the developer to promise that "I will clean up this scope". extend gives you the same use case, but does not require you to make this promise.

dphfox commented 5 months ago

Will implement as :innerScope()