From 9d4af8e669fe095c1f207c6ab77d9a183365ca15 Mon Sep 17 00:00:00 2001 From: littensy <56808540+littensy@users.noreply.github.com> Date: Fri, 28 Nov 2025 20:36:51 -0800 Subject: [PATCH] fix: prevent early reactive gc, alien-signals 3.1.1 --- packages/charm-sync/src/client.luau | 24 ++++--- packages/charm/src/__tests__/gc.spec.luau | 30 ++++++--- .../charm/src/__tests__/reactive.spec.luau | 8 ++- .../charm/src/__tests__/trigger.spec.luau | 16 +++++ packages/charm/src/init.luau | 65 ++++++++++--------- 5 files changed, 94 insertions(+), 49 deletions(-) diff --git a/packages/charm-sync/src/client.luau b/packages/charm-sync/src/client.luau index a51ec55..677e7b9 100644 --- a/packages/charm-sync/src/client.luau +++ b/packages/charm-sync/src/client.luau @@ -8,7 +8,15 @@ type SyncPayload = types.SyncPayload type Setter = (T | (T) -> any) -> () local trackedSignals: { [string]: Setter } = {} -local trackedReactives: { [string]: Setter } = {} +local trackedReactives: { [string]: ReactiveProxy } = {} + +local function shallowMergeReactive(reactive: ReactiveProxy, source: { [any]: any }) + local target = reactive.__target + for key, value in source do + target[key] = value + end + Charm.trigger(reactive.__track) +end --[[ Registers a map of setters to the state with the corresponding keys on the @@ -26,7 +34,7 @@ local function addSignals(signals: { [string]: Setter | {} }) if type(signal) == "function" then trackedSignals[key] = signal else - trackedReactives[key] = ((signal :: any) :: ReactiveProxy).__set + trackedReactives[key] = (signal :: any) :: ReactiveProxy end end end @@ -54,9 +62,10 @@ end local function applyInitPayload(data: { [string]: any }) for key, value in data do - local trackedSignal: Setter? = trackedSignals[key] or trackedReactives[key] - if trackedSignal then - trackedSignal(value) + if trackedSignals[key] then + trackedSignals[key](value) + elseif trackedReactives[key] then + shallowMergeReactive(trackedReactives[key], value) end end end @@ -71,9 +80,8 @@ local function applyPatchPayload(data: { [string]: any }) return patch.apply(value, diffs) end) elseif trackedReactive then - trackedReactive(function(value): () - patch.applyMutable(value, diffs) - end) + patch.applyMutable(trackedReactive.__target, diffs) + Charm.trigger(trackedReactive.__track) end end end diff --git a/packages/charm/src/__tests__/gc.spec.luau b/packages/charm/src/__tests__/gc.spec.luau index 3ab4dfd..d74196e 100644 --- a/packages/charm/src/__tests__/gc.spec.luau +++ b/packages/charm/src/__tests__/gc.spec.luau @@ -66,33 +66,34 @@ test("should not release effect for atom", function() assert(effectCalls == 2, "should call effect again") end) -test("should release reactive tables", function() +test("should release inner reactive proxy", function() local valueRef = weakRef({}) - local src = reactive({ value = valueRef.value }) + local src: any = reactive({ value = valueRef.value }) - effect(function() + local dispose = effect(function() local _ = src.value -- create proxy end) src.value = nil :: any -- release proxy + dispose() gc() - assert(valueRef.value == nil, "should release reactive table") - assert((src :: ReactiveProxy).__signals.value == nil, "should release reactive signals") + assert(src.__signals.value == nil, "should release signals") + assert(valueRef.value == nil, "should release inner proxy") end) test("should release reactive property deps", function() - local src = reactive({ foo = 1 }) + local src: any = reactive({ foo = 1 }) local c = computed(function() return src.foo end) c() - assert((src :: ReactiveProxy).__signals.foo ~= nil, "should have deps for foo") + assert(src.__signals.foo ~= nil, "should have deps for foo") c = nil :: any gc() - assert((src :: ReactiveProxy).__signals.foo == nil, "should release deps for foo") + assert(src.__signals.foo == nil, "should release deps for foo") end) test("should not release effect for reactive", function() @@ -110,4 +111,17 @@ test("should not release effect for reactive", function() assert(calls == 2, "should call effect again") end) +test("should not release tracked reactive", function() + local src = Charm.reactive({ users = {} :: { [string]: {}? } }) + local user + + effect(function() + user = src.users.user + end) + + gc() + src.users.user = {} + assert(user ~= nil, "src.users.user should not be released") +end) + return {} diff --git a/packages/charm/src/__tests__/reactive.spec.luau b/packages/charm/src/__tests__/reactive.spec.luau index 79c2be0..7e11af4 100644 --- a/packages/charm/src/__tests__/reactive.spec.luau +++ b/packages/charm/src/__tests__/reactive.spec.luau @@ -111,14 +111,18 @@ test("should not track unread properties", function() end) test("should overwrite with table passed to setter", function() - local src, setSrc = reactive({ a = 1, b = 2, c = 3 } :: { [string]: number }) + local src, setSrc = reactive({ a = 1, b = 2, c = 3 } :: { [string]: number? }) local calls = {} effect(function() table.insert(calls, `{src.a}{src.b}{src.c}`) end) - setSrc({ a = 2, c = 4 }) + setSrc(function(src) + src.a = 2 + src.b = nil + src.c = 4 + end) assert(table.concat(calls, ",") == "123,2nil4", table.concat(calls, ",")) end) diff --git a/packages/charm/src/__tests__/trigger.spec.luau b/packages/charm/src/__tests__/trigger.spec.luau index 4055cbe..d0b1938 100644 --- a/packages/charm/src/__tests__/trigger.spec.luau +++ b/packages/charm/src/__tests__/trigger.spec.luau @@ -69,6 +69,22 @@ test("should trigger effect once", function() assert(triggers == 2, "effect should run twice") end) +test("should not notify the trigger function sub", function() + local src1 = signal({}) + local src2 = computed(function() + return src1() + end) + + effect(function() + src1() + src2() + end) + trigger(function() + src1() + src2() + end) +end) + test("should trigger updates for subscribe", function() local src = signal({} :: { number }) local triggers = 0 diff --git a/packages/charm/src/init.luau b/packages/charm/src/init.luau index 7cc3998..d7bdc18 100644 --- a/packages/charm/src/init.luau +++ b/packages/charm/src/init.luau @@ -1,7 +1,9 @@ local system = require("@self/system") export type ReactiveFlags = system.ReactiveFlags + export type ReactiveNode = system.ReactiveNode + export type Link = system.Link export type Atom = (...((T) -> T) | T) -> T @@ -9,11 +11,10 @@ export type Atom = (...((T) -> T) | T) -> T export type ReactiveProxy = typeof(setmetatable( {} :: { __track: () -> (), - __set: (T | ((T) -> ())?) -> (), __target: T, __signals: { [unknown]: () -> unknown }, }, - {} + {} :: { __index: T } )) type EffectScopeNode = ReactiveNode & { @@ -73,6 +74,10 @@ local propagate: (link: Link) -> () local checkDirty: (link: Link, sub: ReactiveNode) -> boolean local shallowPropagate: (link: Link) -> () +local ReactiveProxy = { + proxyMap = setmetatable({} :: { [any]: ReactiveProxy }, { __mode = "v" }), +} + local cycle = 0 local batchDepth = 0 local notifyIndex = 0 @@ -80,9 +85,6 @@ local queued: { EffectNode? } = {} local queuedLength = 0 local activeSub: ReactiveNode? -local ReactiveProxy = {} -local proxyMap = setmetatable({} :: { [any]: ReactiveProxy }, { __mode = "v" }) - local function noYield(fn: (Args...) -> Results...): (Args...) -> Results... if not flags.strict then return fn @@ -504,13 +506,15 @@ local function trigger(fn: () -> ()) local prevSub = setActiveSub(sub) local success, result: any = pcall(noYield(fn)) activeSub = prevSub - while sub.deps do - local link = sub.deps :: Link + local link = sub.deps + while link do local dep = link.dep - unlink(link, sub) - if dep.subs then - propagate(dep.subs) - shallowPropagate(dep.subs) + link = unlink(link, sub) + local subs = dep.subs + if subs then + sub.flags = NONE + propagate(subs) + shallowPropagate(subs) end end if batchDepth == 0 then @@ -848,7 +852,7 @@ end ]] local function toRaw(value: T): T if isReactive(value) then - local value: ReactiveProxy = value :: any + local value = value :: ReactiveProxy & T value.__track() return toRaw(value.__target) end @@ -881,42 +885,41 @@ end @return A function for updating the original table. @see https://github.com/littensy/charm?tab=readme-ov-file#reactiveinitialvalue ]] -local function reactive(initialValue: T & {}, trackParent: (() -> ())?): (T, (update: (T) -> () | T?) -> ()) +local function reactive(initialValue: T & {}, trackParent: (() -> ())?): (T, (update: ((T) -> ())?) -> ()) initialValue = toRaw(initialValue) - local proxy = proxyMap[initialValue] + local proxy = ReactiveProxy.proxyMap[initialValue] if not proxy then - local track = computed(function() + local track = computed(function(version: number?) + -- Reference the proxy to avoid GCing while an effect is actively + -- tracking this reactive object. + local _ref = proxy + -- Allow updates from the parent to propagate down when the + -- target is mutated directly through the setter. if trackParent then trackParent() end - return {} + return (version or 0) + 1 end) - local function set(update: (T) -> () | T?) - if type(update) == "function" then - update(initialValue) - elseif type(update) == "table" then - table.clear(initialValue) - for key, value in update :: {} do - (initialValue :: {})[key] = value - end - end - trigger(track) - end - proxy = setmetatable({ __target = initialValue, __signals = setmetatable({}, { __mode = "v" }) :: any, __track = track, - __set = set, }, ReactiveProxy) - proxyMap[initialValue] = proxy + ReactiveProxy.proxyMap[initialValue] = proxy end - return proxy :: any, proxy.__set + local function setValue(update: ((T) -> ())?) + if update then + update(proxy.__target) + end + trigger(proxy.__track) + end + + return proxy :: any, setValue end function ReactiveProxy.__index(self: ReactiveProxy, key: any): any