centau / vide

A reactive Luau library for creating UI.
https://centau.github.io/vide/
MIT License
95 stars 16 forks source link

`show()`/`switch()` not parenting created instances #20

Closed quantix-dev closed 1 year ago

quantix-dev commented 1 year ago

Using the below test script in the studio command bar:

local vide = require(workspace.vide)

local create = vide.create
local read = vide.read
local show = vide.show
local source = vide.source

local function input(_props: { Readonly: () -> boolean })
    local readonly = function()
        print("readonly updated")
        return read(_props.Readonly or false)
    end

    local function ReadonlyInput()
        print("create readonly input")
        return create("TextLabel") {
            BackgroundColor3 = Color3.fromRGB(255, 0, 0),
            Size = UDim2.fromScale(1, 1),
        }
    end

    local function Input()
        print("create input")
        return create("TextBox") {
            BackgroundColor3 = Color3.fromRGB(0, 255, 0),
            Size = UDim2.fromScale(1, 1),
            Text = "Input Box",
        }
    end

    return create("Frame") {
        BackgroundTransparency = 1,
        ClipsDescendants = false,
        Size = UDim2.fromScale(1, 0.5),
        show(readonly, ReadonlyInput, Input),
    }
end

vide.mount(function()
    local readonly = source(false)

    -- update source after 3 seconds
    task.delay(3, function()
        print("update readonly")
        readonly(true)
    end)

    return input({
        Readonly = readonly
    })
end, game.StarterGui)

I noted that, while it was correctly updating show() and creating the components (detected via their prints) it was not properly parenting them in the instance tree.

image Above is a screenshot ofthe generated instances, as you can see only Frame (the top-level component) was being generated whilst neither the TextBox or TextLabel are visible.

See the output of the prints below, image

quantix-dev commented 1 year ago

Further investigation into the vide source seems to be an issue related to the source provided by the switch() function

https://github.com/centau/vide/blob/fd2888afabc7691cb7655c8c0ae219f2eb912c75/src/switch.luau#L61-L64

the above function is returning node.cache, which is nil when the source is called. I added a print to the node cache in that source

return function()
    track(node)
    print("node cache: ", node.cache)
    return node.cache
end
20230926-1831-5S7ODY
quantix-dev commented 1 year ago

Seems like an issue with vide.strict when it's enabled it doesn't work but disabling it works.

When strict is enabled the evaluate_node function of graph calls the effect of switch which internally sets the last_component to be the same as the created component

because of the above, the first equality check of the switch effect https://github.com/centau/vide/blob/fd2888afabc7691cb7655c8c0ae219f2eb912c75/src/switch.luau#L27 is returning the cached value which because it was ran in strict first and strict doesn't set the node.cache meaning the value stays nil and any subsequent calls will result in the nil value being used because it thinks that's the correct cached value

20230926-1841-wMZi6W
centau commented 1 year ago

Thank you for the comprehensive report

centau commented 1 year ago

This fix required a slight change in behavior for strict mode. Before in strict mode when a node is evaluated twice, the initial evaluation result is discarded, and the node's new cached value is set to the second evaluation result. This was to try make it so from the pov of the reactive graph, each node only evaluates once despite it actually evaluating twice.

This new change makes it so the node's cached value is set after both evaluations, and will update its children if the cached value before the double evaluation is different to after the double evaluation.

The end user won't notice any difference with this except from a few subtle cases such as this one:

local src = source(true)

local count = 0

effect(function(x: number)
    src()
    count = x + 1
    return count
end, count)

CHECK(count == 2)
src(not src())
CHECK(count == 4)

Previously, the counts would be 1 then 2, instead of 2 then 4.

I don't know if this change will have implications down the line but we will see.

Normally, the original approach would work fine, but switch() actually breaks Vide's rule of pure computation for the sake of optimization to prevent unnecessarily recreating the component.