diff --git a/README.md b/README.md index a580e50..7747b2d 100644 --- a/README.md +++ b/README.md @@ -542,16 +542,17 @@ max(-1) --> 1 ### `globals` -Global flags that customize the behavior of Charm. +You can customize the behavior of Charm through the `globals` table: + +| Flag | Default | Description | +| ----------------- | -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | +| strict | `true`/`false` | Enforces synchronous, non-yielding behavior in signals, effects, and other critical code. | +| frozen | `true`/`false` | Enforces data immutability by deep-freezing tables passed to signals. | +| trackInnerEffects | `true` | Whether nested effects should be tracked and cleaned up when the parent effect re-runs. This should only be disabled to debug issues during migration. | +| allowRecursion | `false` | Allows recursion in effects and computed signals, which is intentionally disallowed in the [alien-signals](https://github.com/stackblitz/alien-signals) reactive system. | The `strict` and `frozen` flags are automatically enabled when the Luau optimization level is less than `2`, which is true in Roblox Studio. -| Flag | Default | Description | -| ----------------- | -------------- | ------------------------------------------------------------------------------------------------------------------------------------------------- | -| strict | `true`/`false` | Enforces synchronous, non-yielding behavior in signals, effects, and other critical code. | -| frozen | `true`/`false` | Enforces data immutability by deep-freezing tables passed to signals. | -| trackInnerEffects | `true` | Whether nested effects should be tracked and cleaned up when the parent effect re-runs. Should only be disabled to debug issues during migration. | - --- ## Client-Server Sync @@ -830,14 +831,17 @@ Charm v0.11 introduces _a lot_ of breaking changes, so below are some tips that 4. Nested effects automatically clean up when the parent effect re-runs or gets disposed. In other words, all effects created during the execution of another effect will be added as a "child" and clean up with the parent effect. This might cause issues in code that relied on the old behavior, where effects were detached from the parent. - This feature applies to all reaction APIs, including the listener function in `subscribe()` and the observer function in `observe()`. - Effects that should not be tracked by a parent effect/scope should be wrapped in [`untracked()`](#untrackedcallback). - - This feature can cause runtime bugs that are hard to track! If you need to narrow down a cause, you can disable this feature by setting [`globals.trackInnerEffects`](#globals) to `false`. + - This feature can introduce runtime bugs in legacy code. If you need to narrow down a cause, you can disable this feature by setting [`globals.trackInnerEffects`](#globals) to `false`. > [!NOTE] > An example of nested effects causing a bug is an old implementation of [`VideCharm.useAtom`](./packages/vide-charm/src/init.luau) that did not wrap the source update in `untracked()`. Because Vide effects run immediately after a source updates, Vide will notify components during the execution of the Charm effect in `useAtom`. > > This meant effects created as a side effect of a source update would implicitly get added as a child of the `useAtom` effect, and they could get disposed at the wrong time and desync UI. -5. Consider refactoring your code to use some new quality-of-life features. Many of these are made possible with many thanks to [alien-signals](https://github.com/stackblitz/alien-signals)! +5. Recursion is disallowed in effects and computed signals now that Charm uses the [alien-signals algorithm](https://github.com/stackblitz/alien-signals). This change may introduce bugs in code relying on the old behavior. + - You can disable recursion checks by setting [`globals.allowRecursion`](#globals) to `true`, but try to consider other options first, as recursion isn't fully supported and is not well optimized. + +6. Consider refactoring your code to use some new quality-of-life features. Many of these are made possible thanks to [alien-signals](https://github.com/stackblitz/alien-signals)! - Added [`signal()`](#signalinitialvalue-equals) to make reads and writes more explicit - Added [`listen()`](#listengetter-callback) for running a subscription once immediately - Added [`effectScope()`](#effectscopecallback) for collecting and cleaning up multiple effects at once @@ -845,7 +849,6 @@ Charm v0.11 introduces _a lot_ of breaking changes, so below are some tips that - The [`computed()`](#computedgetter) callback now gets called with the previous computed value - Optimized `computed()` to use lazy evaluation instead of eager updates - Optimized `mapped()` to only map values that changed in the original table - - Effect execution and ordering is more reliable --- diff --git a/packages/charm/src/__tests__/effect.spec.luau b/packages/charm/src/__tests__/effect.spec.luau index b1ba2bf..071d2a0 100644 --- a/packages/charm/src/__tests__/effect.spec.luau +++ b/packages/charm/src/__tests__/effect.spec.luau @@ -269,6 +269,27 @@ test("should handle flags are indirectly updated during checkDirty", function() assert(triggers == 2, "did not update indirectly") end) +test("should handle effect recursion for the first execution", function() + local src1, setSrc1 = signal(0) + local src2, setSrc2 = signal(0) + + local triggers1 = 0 + local triggers2 = 0 + + effect(function() + triggers1 += 1 + setSrc1(math.min(src1() + 1, 5)) + end) + effect(function() + triggers2 += 1 + setSrc2(math.min(src2() + 1, 5)) + src2() + end) + + assert(triggers1 == 1, `bad triggers1: {triggers1}`) + assert(triggers2 == 1, `bad triggers2: {triggers2}`) +end) + test("should run cleanups in sequence", function() local order = {} diff --git a/packages/charm/src/__tests__/observe.spec.luau b/packages/charm/src/__tests__/observe.spec.luau index 01add3d..3f9a835 100644 --- a/packages/charm/src/__tests__/observe.spec.luau +++ b/packages/charm/src/__tests__/observe.spec.luau @@ -90,6 +90,29 @@ test("should queue addition and removal", function() assert(table.concat(edits) == "+1+2+3+4-2-3-4", `bad edits: {table.concat(edits)}`) end) +test("should handle recursive additions and removals", function() + local items, setItems = signal({ 1 }) + local edits = {} + + observe(items, function(item) + if item < 5 then + setItems(function(state) + state = table.clone(state) + table.insert(state, item + 1) + return state + end) + else + setItems({}) + end + table.insert(edits, `+{item}`) + return function() + table.insert(edits, `-{item}`) + end + end) + + assert(table.concat(edits) == "+1+2+3+4+5-1-2-3-4-5", `bad edits: {table.concat(edits)}`) +end) + test("should handle self removal", function() local items, setItems = signal({ 1, 2, 3, 4 }) local edits = {} diff --git a/packages/charm/src/alien.luau b/packages/charm/src/alien.luau index 498b8b9..5a610e2 100644 --- a/packages/charm/src/alien.luau +++ b/packages/charm/src/alien.luau @@ -1,3 +1,4 @@ +local globals = require("./globals") --[[ Reactive system adapted from alien-signals at https://github.com/stackblitz/alien-signals/blob/master/src/system.ts @@ -167,10 +168,17 @@ local function createReactiveSystem(props: { end local function propagate(link: Link): () + local allowRecursion = globals.allowRecursion + repeat local sub = link.sub local flags = sub.flags + if allowRecursion then + flags = bit32.band(flags, bit32.bnot(bit32.bor(RECURSED, RECURSED_CHECK))) + sub.flags = flags + end + if bit32.band(flags, bit32.bor(RECURSED_CHECK, RECURSED, DIRTY, PENDING)) == 0 then sub.flags = bit32.bor(flags, PENDING) elseif bit32.band(flags, bit32.bor(RECURSED_CHECK, RECURSED)) == 0 then @@ -185,7 +193,7 @@ local function createReactiveSystem(props: { flags = NONE end - if bit32.btest(flags, WATCHING) then + if bit32.band(flags, bit32.bor(WATCHING, RECURSED_CHECK)) == WATCHING then notify(sub) end diff --git a/packages/charm/src/core.luau b/packages/charm/src/core.luau index 48d545d..cdcb6ac 100644 --- a/packages/charm/src/core.luau +++ b/packages/charm/src/core.luau @@ -51,7 +51,7 @@ type EffectScope = ReactiveNode & { } type Effect = ReactiveNode & { - fn: () -> (() -> ())?, + fn: () -> ...(() -> ())?, cleanups: { () -> () }, } @@ -116,7 +116,7 @@ local function run(e: Effect): () e.depsTail = nil e.flags = bit32.bor(WATCHING, RECURSED_CHECK) local prevSub = setActiveSub(e) - local success, result: (() -> ())? = (if globals.strict then noYield else pcall)(e.fn) + local success, result = (if globals.strict then noYield else pcall)(e.fn) activeSub = prevSub e.flags = bit32.band(e.flags, bit32.bnot(RECURSED_CHECK)) purgeDeps(e) @@ -162,7 +162,7 @@ local function updateComputed(c: Computed): boolean c.flags = bit32.bor(MUTABLE, RECURSED_CHECK) local prevSub = setActiveSub(c) local oldValue = c.value - local success, result: unknown = (if globals.strict then noYield else pcall)(c.getter, oldValue) + local success, result = (if globals.strict then noYield else pcall)(c.getter, oldValue) activeSub = prevSub c.flags = bit32.band(c.flags, bit32.bnot(RECURSED_CHECK)) purgeDeps(c) @@ -209,10 +209,11 @@ local function computedOper(this: Computed): () -> T end end elseif flags == NONE then - this.flags = MUTABLE + this.flags = bit32.bor(MUTABLE, RECURSED_CHECK) local prevSub = setActiveSub(this) local success, result = pcall(this.getter) activeSub = prevSub + this.flags = bit32.band(this.flags, bit32.bnot(RECURSED_CHECK)) if success then this.value = result else @@ -235,7 +236,7 @@ local function signalOper( local value: T if type(update) == "function" then local prevSub = setActiveSub(nil) - local success, result: T = (if globals.strict then noYield else pcall)(update, this.pendingValue) + local success, result = (if globals.strict then noYield else pcall)(update, this.pendingValue) activeSub = prevSub if not success then error(result) @@ -365,18 +366,19 @@ end --- @param fn The function to run when dependencies change. --- @return A function for disposing the effect. --- @see https://github.com/littensy/charm?tab=readme-ov-file#effectcallback -local function effect(fn: () -> ()): () -> () +local function effect(fn: () -> ...(() -> ())?): () -> () local e: Effect = { fn = fn, - flags = WATCHING, + flags = bit32.bor(WATCHING, RECURSED_CHECK), cleanups = {}, } local prevSub = setActiveSub(e) if prevSub and (globals.trackInnerEffects or prevSub.flags == NONE) then link(e, prevSub, 0) end - local success, result: (() -> ())? = (if globals.strict then noYield else pcall)(fn) + local success, result = (if globals.strict then noYield else pcall)(fn) activeSub = prevSub + e.flags = bit32.band(e.flags, bit32.bnot(RECURSED_CHECK)) if not success then error(result) elseif result then @@ -392,7 +394,7 @@ end --- @param detached If true, the scope will not be automatically disposed of when the parent scope is disposed. Defaults to false. --- @return A function for disposing all effects created within the scope. --- @see https://github.com/littensy/charm?tab=readme-ov-file#effectscopecallback-detached -local function effectScope(fn: () -> (), detached: boolean?): () -> () +local function effectScope(fn: () -> ...(() -> ())?, detached: boolean?): () -> () local e: EffectScope = { flags = NONE, cleanups = {}, @@ -401,7 +403,7 @@ local function effectScope(fn: () -> (), detached: boolean?): () -> () if prevSub and not detached then link(e, prevSub, 0) end - local success, result: (() -> ())? = (if globals.strict then noYield else pcall)(fn) + local success, result = (if globals.strict then noYield else pcall)(fn) activeSub = prevSub if not success then error(result) diff --git a/packages/charm/src/globals.luau b/packages/charm/src/globals.luau index af6a381..26273d9 100644 --- a/packages/charm/src/globals.luau +++ b/packages/charm/src/globals.luau @@ -17,4 +17,8 @@ return { --- when the parent effect re-runs. This is usually desirable, but can be --- disabled to revert to the old behavior. Defaults to `true`. trackInnerEffects = true, + --- Whether to allow recursive updates in effects and computed signals. + --- Disabling this can help catch certain classes of bugs, but may also + --- cause valid code to error. Defaults to `false`. + allowRecursion = false, } diff --git a/packages/charm/src/index.d.ts b/packages/charm/src/index.d.ts index de9c2c4..8400040 100644 --- a/packages/charm/src/index.d.ts +++ b/packages/charm/src/index.d.ts @@ -36,6 +36,12 @@ export const globals: { * disabled to revert to the old behavior. Defaults to `true`. */ trackInnerEffects: boolean; + /** + * Whether to allow recursive updates in effects and computed signals. + * Disabling this can help catch certain classes of bugs, but may also + * cause valid code to error. Defaults to `false`. + */ + allowRecursion: boolean; }; /** diff --git a/packages/charm/src/observe.luau b/packages/charm/src/observe.luau index 244b08b..d169ac3 100644 --- a/packages/charm/src/observe.luau +++ b/packages/charm/src/observe.luau @@ -42,16 +42,19 @@ local function observe(getter: () -> { [K]: V }, callback: (item: V, key: return effectScope(function() effect(function() - -- Batch so that side effects run after all scopes are created. This - -- ensures that a scope creating or removing items doesn't interfere - -- with the current update cycle. - batch(updateScopes, getter()) + local data = getter() - -- Effects can't trigger themselves more than once, so if a scope - -- creates or removes items, we need to check for updates again. The - -- way we do this abuses a weird property of signal accesses that - -- causes the effect to re-run. - getter() + -- Batch so that side effects run after all scopes are created. + -- This ensures that a scope creating or removing items doesn't + -- interfere with the current update cycle. + batch(updateScopes, data) + + -- Effects can't trigger themselves, so if a scope creates or + -- removes items, we need to check for updates again. + local newData = getter() + if newData ~= data then + batch(updateScopes, newData) + end end) return function()