fix(core): dispose internal/update hooks with the fiber - #56
Open
tdwhere123 wants to merge 2 commits into
Open
Conversation
Non-global internal/update listeners skipped fiber.effect registration, so reloads left stale closures in fiber._hooks. Register them as effects so unload drops the previous generation.
…fect fiber.effect already treats the callback return as the disposer; DisposableList.push is that remover, so the extra binding is noise.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Non-global
ctx.on('internal/update')is intercepted inEventsServiceand stored onfiber._hooks, thenon()returns early so the listener never becomes a fiber effect._unloadonly clears_disposables. The Fiber instance is reused across reloads, so eachfiber.update()keeps the previous generation's closures.Three config updates on current
maintherefore dispatch1 + 2 + 3listeners:mainupdate({ n: 1/2/3 })[1, 2, 2, 3, 3, 3][1, 2, 3]dispose()before update{ global: true }listenerThe accumulation regressions fail on
8cc9e33with those extra calls and pass on this branch.Change
Register the special-cased listener through
fiber.effect, so_unloadalready drops it. Do not clearfiber._hooksand do not touch_unload(that path is being rewritten in #39).This PR is independent of #55 (
internal/statusisolation) and does not changeregister()/_resolve(#51).Regression coverage
packages/core/tests/update-hooks.spec.tscovers accumulation across reloads, explicit dispose of the current generation, and that globalinternal/updatehooks are not bound to a child fiber's reload.Validation
yarn lintyarn yakumo tscyarn test cordis/update-hooks— 3 tests passedyarn test cordis— 13 files, 74 tests passedyarn test:json— 20 files, 166 tests passedgit diff --check origin/main...HEAD— passed