diff --git a/CHANGELOG.md b/CHANGELOG.md index 441d38b2..6e72f4da 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,21 @@ # Changelog +## v3.5.4 - 2026-06-25 + +### Fixed + +- **Effects were dropped when a component used render-phase updates (regression in + v3.5.3).** The render-phase convergence loop cleared the fiber's per-iteration + effect list before each re-run; combined with the deps-equality check (a + stable-deps effect does not re-register when its deps are unchanged), the effect + ended up registered in no iteration and never ran. The loop no longer clears the + effect list — effects accumulate across convergence iterations and are then + de-duplicated by cleanup-index (keeping the last, i.e. the final render's effect + per slot). Each effect now runs exactly once with the converged state, for both + stable and changing deps, and multiple effects run once each in declaration + order. Covered by `TestRenderPhaseEffectRunsOnceStableDeps` / + `...ChangingDeps` / `TestRenderPhaseMultipleEffectsEachOnce`. + ## v3.5.3 - 2026-06-25 ### Fixed diff --git a/examples/public-examples-site/assets/docs/latest-release-notes.md b/examples/public-examples-site/assets/docs/latest-release-notes.md index 0ef4a482..0754d512 100644 --- a/examples/public-examples-site/assets/docs/latest-release-notes.md +++ b/examples/public-examples-site/assets/docs/latest-release-notes.md @@ -3,18 +3,19 @@ This docs-site page mirrors the first section returned by `tools/changelogcheck.LatestEntry(CHANGELOG.md)`. -## v3.5.3 - 2026-06-25 +## v3.5.4 - 2026-06-25 -The current latest changelog section is `v3.5.3 - 2026-06-25`. **Fixed:** a -render-phase state update (a component calling its own `setState` during render — -the derived-state pattern) left the DOM showing a value the component never -actually rendered and never converged. `renderFunctionComponent` now detects a -render-phase update (via a new `activeRenderFiber` flag, distinct from the ambient -current fiber so it never trips on the SSR path or hook unit tests) and re-runs -the component to convergence — bounded at 25 iterations with a diagnostic to guard -against an unconditional-setState infinite loop — matching React's "keeps -restarting until there are no more new updates." Found while bug-hunting GWC's -state management against React. It is checked by the blocking `tools/changelogcheck` -release gate before a versioned release can ship. +The current latest changelog section is `v3.5.4 - 2026-06-25`. **Fixed:** a +regression in v3.5.3 where effects were dropped when a component used render-phase +updates. The convergence loop cleared the fiber's per-iteration effect list before +each re-run; combined with the deps-equality check (a stable-deps effect does not +re-register when its deps are unchanged), the effect ended up registered in no +iteration and never ran. The loop no longer clears the effect list — effects +accumulate across iterations and are then de-duplicated by cleanup-index (keeping +the final render's effect per slot), so each effect runs exactly once with the +converged state (for both stable and changing deps), and multiple effects run once +each in declaration order. Found by stress-testing the v3.5.3 fix. It is checked by +the blocking `tools/changelogcheck` release gate before a versioned release can +ship. See the repository root `CHANGELOG.md` for the full entry body. diff --git a/internal/runtime/hot_reload.go b/internal/runtime/hot_reload.go index b7acc8d4..0fd95a9b 100644 --- a/internal/runtime/hot_reload.go +++ b/internal/runtime/hot_reload.go @@ -911,8 +911,16 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo // this render (parseFiber.renderPhaseUpdate, set by the hook setter), re-run // it with the new state until it stops updating — bounded like React's // re-render limit — so the committed output reflects the final state instead - // of a half-rendered intermediate. + // of a half-rendered intermediate. The hook indices are reset per iteration + // but the deps/cleanups/effects arrays persist; effects accumulate across + // iterations (the deps-equality check makes a stable effect register only + // once, while a deps-changing effect registers per iteration), so they are + // de-duplicated by cleanup index afterwards to commit exactly the final + // render's effect per slot. NOTE: do not clear parseFiber.effects here — + // clearing it loses an effect whose deps did not change across iterations. + parseConverged := false for parseRPIters := 0; parseFiber.renderPhaseUpdate; parseRPIters++ { + parseConverged = true parseFiber.renderPhaseUpdate = false if parseRPIters >= 25 { ReportDiagnosticWithContext("runtime", DiagnosticError, @@ -934,9 +942,6 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo parseFiber.hooks.cleanupIndex = 0 parseFiber.hooks.signature = parseFiber.hooks.signature[:0] } - if parseFiber.effects != nil { - parseFiber.effects = parseFiber.effects[:0] - } SetCurrentFiber(parseFiber) parseRt.activeRenderFiber = parseFiber func() { @@ -954,6 +959,9 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo } }() } + if parseConverged { + parseFiber.effects = dedupRenderPhaseEffects(parseFiber.effects) + } if parseAttempt == 0 && parseRestore != nil { if !componentSnapshotSerializableCompatible(parseRestore, parseFiber) { @@ -975,3 +983,33 @@ func (parseRt *Runtime) renderFunctionComponent(parseFiber *Fiber) (*Element, bo return nil, false, nil } + +// dedupRenderPhaseEffects collapses the effects accumulated across render-phase +// convergence iterations to exactly one per cleanup-index slot — the last one, +// i.e. the effect produced by the final converged render — preserving +// declaration (cleanup-index) order. Without this, an effect whose deps change as +// state converges would be registered (and run) once per iteration. +func dedupRenderPhaseEffects(parseEffects []Effect) []Effect { + if len(parseEffects) <= 1 { + return parseEffects + } + parseLastBySlot := make(map[int]Effect, len(parseEffects)) + parseMaxSlot := -1 + for _, parseEffect := range parseEffects { + parseLastBySlot[parseEffect.CleanupIndex] = parseEffect + if parseEffect.CleanupIndex > parseMaxSlot { + parseMaxSlot = parseEffect.CleanupIndex + } + } + if len(parseLastBySlot) == len(parseEffects) { + // No duplicates (every slot appeared once); keep the original order. + return parseEffects + } + parseOut := parseEffects[:0] + for parseSlot := 0; parseSlot <= parseMaxSlot; parseSlot++ { + if parseEffect, parseOk := parseLastBySlot[parseSlot]; parseOk { + parseOut = append(parseOut, parseEffect) + } + } + return parseOut +} diff --git a/ui/render_phase_effects_test.go b/ui/render_phase_effects_test.go new file mode 100644 index 00000000..e220f8f8 --- /dev/null +++ b/ui/render_phase_effects_test.go @@ -0,0 +1,101 @@ +//go:build !js || !wasm + +package ui_test + +import ( + "fmt" + "testing" + + "github.com/monstercameron/GoWebComponents/internal/platform/mockdom" + "github.com/monstercameron/GoWebComponents/internal/runtime" +) + +// Regression test for v3.5.4: render-phase convergence (v3.5.3) must commit +// exactly the final render's effects — running each effect once, with the +// converged state — for both stable and changing deps. The v3.5.3 convergence +// loop cleared the per-iteration effect list, which (combined with the +// deps-equality check) dropped a stable-deps effect entirely. +func TestRenderPhaseEffectRunsOnceStableDeps(t *testing.T) { + adapter := mockdom.NewMockDOMAdapter() + rt := runtime.NewRuntime(runtime.Config{DOMAdapter: adapter, Reset: true}) + root := adapter.CreateElement("div") + + parseEffectRuns := 0 + var parseSaw []int + rt.RenderInto(root, runtime.CreateElement(func() *runtime.Element { + parseGet, parseSet := runtime.GoUseState(rt, 0) + if parseGet() < 3 { + parseSet(parseGet() + 1) + } + runtime.GoUseEffect(func() func() { + parseEffectRuns++ + parseSaw = append(parseSaw, parseGet()) + return nil + }, 0) // stable deps + return runtime.CreateElement("span", map[string]any{}, fmt.Sprint(parseGet())) + }, map[string]any{})) + + if parseEffectRuns != 1 { + t.Errorf("effect runs = %d, want 1 (only the final converged render), saw=%v", parseEffectRuns, parseSaw) + } + if len(parseSaw) != 1 || parseSaw[0] != 3 { + t.Errorf("effect observed %v, want [3] (the converged state)", parseSaw) + } +} + +func TestRenderPhaseEffectRunsOnceChangingDeps(t *testing.T) { + adapter := mockdom.NewMockDOMAdapter() + rt := runtime.NewRuntime(runtime.Config{DOMAdapter: adapter, Reset: true}) + root := adapter.CreateElement("div") + + parseEffectRuns := 0 + var parseSaw []int + rt.RenderInto(root, runtime.CreateElement(func() *runtime.Element { + parseGet, parseSet := runtime.GoUseState(rt, 0) + if parseGet() < 3 { + parseSet(parseGet() + 1) + } + runtime.GoUseEffect(func() func() { + parseEffectRuns++ + parseSaw = append(parseSaw, parseGet()) + return nil + }, parseGet()) // deps change as state converges + return runtime.CreateElement("span", map[string]any{}, fmt.Sprint(parseGet())) + }, map[string]any{})) + + if parseEffectRuns != 1 { + t.Errorf("changing-deps effect runs = %d, want 1 (deduped to the final render), saw=%v", parseEffectRuns, parseSaw) + } + if len(parseSaw) != 1 || parseSaw[0] != 3 { + t.Errorf("changing-deps effect observed %v, want [3]", parseSaw) + } +} + +// Two effects in a render-phase-converging component: each runs once, in order, +// with the converged state. +func TestRenderPhaseMultipleEffectsEachOnce(t *testing.T) { + adapter := mockdom.NewMockDOMAdapter() + rt := runtime.NewRuntime(runtime.Config{DOMAdapter: adapter, Reset: true}) + root := adapter.CreateElement("div") + + var parseLog []string + rt.RenderInto(root, runtime.CreateElement(func() *runtime.Element { + parseGet, parseSet := runtime.GoUseState(rt, 0) + if parseGet() < 2 { + parseSet(parseGet() + 1) + } + runtime.GoUseEffect(func() func() { + parseLog = append(parseLog, fmt.Sprintf("a:%d", parseGet())) + return nil + }, 0) + runtime.GoUseEffect(func() func() { + parseLog = append(parseLog, fmt.Sprintf("b:%d", parseGet())) + return nil + }, 0) + return runtime.CreateElement("span", map[string]any{}, fmt.Sprint(parseGet())) + }, map[string]any{})) + + if len(parseLog) != 2 || parseLog[0] != "a:2" || parseLog[1] != "b:2" { + t.Errorf("two effects log = %v, want [a:2 b:2] (each once, converged, in order)", parseLog) + } +}