Repository navigation
fix(reactivity): preserve effect options (fix #16100) - #16103
JoaoMarcos44 wants to merge 3 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
RED: four scoped getter cleanup tests fail on 7a65ab8 with zero onStop calls; three controls pass for frozen single-read getters and inherited/non-enumerable callbacks. Tests ran in an isolated snapshot with one worker.
Install onStop and scheduler before evaluating other option getters. Copy own enumerable keys, including symbols, without mutating options or rereading callback getters. Preserve getter receivers and inherited/non-enumerable behavior. GREEN: 84 effect tests and 500 reactivity tests including review probes pass, with 2 existing todo tests. Full TypeScript, targeted lint/format and development/production esm-browser builds pass in the isolated snapshot.
|
Closing in favor of #16105 |
Closes #16100.
Cause and consequence
effect()changed the caller'sschedulerandonStopoptions toundefined. Reusing the options could lose scheduling and cleanup callbacks, and frozen options could throw in ESM. Reading the remaining options before installing cleanup also let a debug-hook getter stop the effect's scope or throw beforeonStopwas attached.Solution
Read
schedulerandonStoponce and install their behavior first. Then copy the other own enumerable option properties to the effect, including symbol keys, without changing the caller's object. Property reads keep their original receiver.Why this approach
It preserves the callback setup order while supporting shared, frozen, readonly and getter-based options. If an
onTrackoronTriggergetter stops the scope, or throws before the caller stops it, cleanup is already installed. Inherited and non-enumerable debug-hook behavior follows the existing option-copying rules.Pros and cons
The PR targets
minorbecause the original mutation regression is in the 3.6 line; stable 3.5.44 andmainretain the non-mutating implementation.Validation
effect.spec.ts: 84/84. Reactivity tests includingunit-gcand supplementary review cases: 500 passed, 2 existing todo tests.effect.ts: 90.26% statements, 88.33% branches, 90.47% functions, 90.90% lines.Original reproduction · PR preview
AI-generated conceptual illustration: reactive effects can reuse the caller-owned options while scheduling and cleanup callbacks are installed first. This is not execution or test evidence.