diff --git a/packages/charm/src/init.luau b/packages/charm/src/init.luau index 0ba77d7..d5f7f6e 100644 --- a/packages/charm/src/init.luau +++ b/packages/charm/src/init.luau @@ -52,6 +52,8 @@ local RECURSED = 0b001000 -- Has been visited in a recursion check local DIRTY = 0b010000 -- Value is outdated and needs recomputation local PENDING = 0b100000 -- Might be dirty, pending verification +local HAS_CHILD = 0b1000000 -- Dependencies include child effects or scopes + -- Global version counter for dependency link deduplication. Incremented at the -- start of each effect/computed run to ensure links created in the same cycle -- are deduplicated. @@ -171,6 +173,33 @@ local function purgeDeps(sub: ReactiveNode) end end +--[[ + Unlinks child effects and effect scopes before a parent effect reruns. +]] +local function unlinkChildEffects(sub: ReactiveNode) + local current = sub.depsTail + while current do + local previous = current.prevDep + local dep = current.dep + if not bit32.btest(dep.flags, MUTABLE) then + unlink(current, sub) + end + current = previous + end +end + +--[[ + Unlinks all dependencies in LIFO order (newest to oldest). +]] +local function disposeAllDepsInReverse(sub: ReactiveNode) + local current = sub.depsTail + while current do + local previous = current.prevDep + unlink(current, sub) + current = previous + end +end + --[[ Runs all cleanup functions registered with the effect node. Any errors thrown during cleanup are caught and logged in a single error after all @@ -209,6 +238,10 @@ local function run(effect: EffectNode) bit32.btest(effectFlags, DIRTY) or (bit32.btest(effectFlags, PENDING) and checkDirty(effect.deps :: Link, effect)) then + if bit32.btest(effectFlags, HAS_CHILD) then + unlinkChildEffects(effect) + end + runCleanups(effect.cleanups) -- Cancel the run if the effect was disposed during cleanup @@ -238,7 +271,7 @@ local function run(effect: EffectNode) runCleanups(effect.cleanups) end else - effect.flags = WATCHING + effect.flags = bit32.bor(WATCHING, bit32.band(effectFlags, HAS_CHILD)) end end @@ -341,15 +374,14 @@ end inactive, with all cleanup functions run and resources released. ]] local function stopEffect(effect: EffectScopeNode) - effect.depsTail = nil effect.flags = NONE - purgeDeps(effect) + disposeAllDepsInReverse(effect) -- Unlink from the parent effect, if it exists local sub = effect.subs if sub then - unlink(sub, effect) + unlink(sub) end runCleanups(effect.cleanups) @@ -413,9 +445,8 @@ local function unwatched(node: ReactiveNode) if bit32.band(node.flags, MUTABLE) == 0 then stopEffect(node :: EffectScopeNode) elseif node.depsTail then - node.depsTail = nil node.flags = bit32.bor(MUTABLE, DIRTY) - purgeDeps(node) + disposeAllDepsInReverse(node) end end @@ -614,8 +645,10 @@ local function effect(fn: () -> ...Cleanup?): Cleanup -- Inner effects are tracked as a dependency of the parent effect, so that -- they can be automatically cleaned up when the parent effect re-runs, and -- to ensure the correct execution order when nested effects are notified - if activeSub and (flags.trackInnerEffects or activeSub.flags == NONE) then - link(node, activeSub, 0) + local parent = activeSub + if parent and (flags.trackInnerEffects or bit32.band(parent.flags, bit32.bnot(HAS_CHILD)) == NONE) then + link(node, parent, 0) + parent.flags = bit32.bor(parent.flags, HAS_CHILD) end -- Start a batch so that the effect can register a cleanup function before @@ -656,8 +689,10 @@ local function effectScope(fn: () -> ...Cleanup?, detached: boolean?): Cleanup } -- Inner effect scopes are tracked as a dependency of the parent effect - if activeSub and not detached then - link(node, activeSub, 0) + local parent = activeSub + if parent and not detached then + link(node, parent, 0) + parent.flags = bit32.bor(parent.flags, HAS_CHILD) end local prevSub = setActiveSub(node) diff --git a/packages/charm/test/effect.test.luau b/packages/charm/test/effect.test.luau index 17cd0a0..da00a52 100644 --- a/packages/charm/test/effect.test.luau +++ b/packages/charm/test/effect.test.luau @@ -521,15 +521,59 @@ test("should effects dispose in order", function() assert(#order == 0, "should not cleanup immediately") setSrc(true) - assert(table.concat(order) == "123", `bad rerun order: {table.concat(order)}`) + assert(table.concat(order) == "321", `bad rerun order: {table.concat(order)}`) table.clear(order) stopEffect() - assert(table.concat(order) == "123", `bad dispose order: {table.concat(order)}`) + assert(table.concat(order) == "321", `bad dispose order: {table.concat(order)}`) table.clear(order) setSrc(false) assert(#order == 0, "should not cleanup after dispose") end) +test("should cleanup nested effects deepest first", function() + local order: { string } = {} + + local stopEffect = effect(function() + effect(function() + effect(function() + return function() + table.insert(order, "grandchild") + end + end) + return function() + table.insert(order, "child") + end + end) + return function() + table.insert(order, "outer") + end + end) + + stopEffect() + assert(table.concat(order, ", ") == "grandchild, child, outer", `bad dispose order: {table.concat(order, ", ")}`) +end) + +test("should cleanup computed child effects in reverse when unwatched", function() + local order: { number } = {} + local c = computed(function() + for i = 1, 3 do + effect(function() + return function() + table.insert(order, i) + end + end) + end + return 0 + end) + + local stopEffect = effect(function() + c() + end) + + stopEffect() + assert(table.concat(order) == "321", `bad dispose order: {table.concat(order)}`) +end) + test("should handle inner effect clearing outer effect's deps on rerun", function() local src, setSrc = signal(false) local stopEffect: () -> () @@ -545,7 +589,7 @@ test("should handle inner effect clearing outer effect's deps on rerun", functio setSrc(true) end) -test.skip("should handle inner effect cleanup disposing outer effect", function() +test("should handle inner effect cleanup disposing outer effect", function() local order: { number } = {} local src, setSrc = signal(false) @@ -566,28 +610,65 @@ test.skip("should handle inner effect cleanup disposing outer effect", function( table.clear(order) setSrc(true) - assert(table.concat(order) == "1234", `bad rerun order: {table.concat(order)}`) + assert(table.concat(order) == "4321", `bad rerun order: {table.concat(order)}`) table.clear(order) setSrc(false) assert(#order == 0, "should not cleanup after dispose") end) -test.skip("should old inner effect dispose before outer effect re-runs", function() +test("should cleanup child before parent on rerun", function() local order: { string } = {} local src, setSrc = signal(false) effect(function() src() - table.insert(order, "outer") + table.insert(order, "outer run") effect(function() + table.insert(order, "inner run") return function() - table.insert(order, "inner") + table.insert(order, "inner cleanup") end end) + return function() + table.insert(order, "outer cleanup") + end end) + table.clear(order) setSrc(true) - assert(table.concat(order) == "innerouter", `bad rerun order: {table.concat(order)}`) + assert( + table.concat(order, ", ") == "inner cleanup, outer cleanup, outer run, inner run", + `bad rerun order: {table.concat(order, ", ")}` + ) +end) + +test("should preserve child tracking after an inner-only rerun", function() + local order: { string } = {} + local outerSrc, setOuterSrc = signal(false) + local innerSrc, setInnerSrc = signal(false) + + effect(function() + outerSrc() + table.insert(order, "outer run") + effect(function() + innerSrc() + table.insert(order, "inner run") + return function() + table.insert(order, "inner cleanup") + end + end) + return function() + table.insert(order, "outer cleanup") + end + end) + + setInnerSrc(true) + table.clear(order) + setOuterSrc(true) + assert( + table.concat(order, ", ") == "inner cleanup, outer cleanup, outer run, inner run", + `bad rerun order: {table.concat(order, ", ")}` + ) end) return {} diff --git a/packages/charm/test/effectScope.test.luau b/packages/charm/test/effectScope.test.luau index 964b7d5..fb2c627 100644 --- a/packages/charm/test/effectScope.test.luau +++ b/packages/charm/test/effectScope.test.luau @@ -92,4 +92,67 @@ test("should dispose inner effects if created in an effect", function() assert(triggers == 2, "ran after dispose") end) +test("should dispose child effects in reverse order", function() + local order: { number } = {} + local stopScope = effectScope(function() + for i = 1, 3 do + effect(function() + return function() + table.insert(order, i) + end + end) + end + end) + + stopScope() + assert(table.concat(order) == "321", `bad dispose order: {table.concat(order)}`) +end) + +test("should dispose nested effects deepest first", function() + local order: { string } = {} + local stopScope = effectScope(function() + effect(function() + effect(function() + return function() + table.insert(order, "grandchild") + end + end) + return function() + table.insert(order, "child") + end + end) + end) + + stopScope() + assert(table.concat(order, ", ") == "grandchild, child", `bad dispose order: {table.concat(order, ", ")}`) +end) + +test("should cleanup an intermediate scope before its parent effect", function() + local order: { string } = {} + local src, setSrc = signal(false) + + effect(function() + src() + table.insert(order, "outer run") + effectScope(function() + effect(function() + table.insert(order, "inner run") + return function() + table.insert(order, "inner cleanup") + end + end) + end) + return function() + table.insert(order, "outer cleanup") + end + end) + + table.clear(order) + setSrc(true) + assert( + table.concat(order, ", ") == "inner cleanup, outer cleanup, outer run, inner run", + `bad rerun order: {table.concat(order, ", ")}` + ) +end) + return {}