Make the rendering test context tracked - #21563
Closed
NullVoxPopuli wants to merge 1 commit into
Closed
Conversation
Test state reaches templates through the top-level component, and tests mutate it with `set(this.context, ...)`. That drives a rerender only because a non-tracked property read still entangles a per-property tag. That read path is legacy: RFC 957's end state removes it, and the async scheduler work needs it gone. Declaring the context properties tracked keeps every existing `set(this.context, ...)` call reactive under modern semantics, so the suite stops depending on the legacy path without touching the 771 call sites. Values that are already decorators (computed properties, injections) define their own reactivity and pass through untouched. No behaviour change on main: 9448 tests, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Contributor
Author
|
this is garbage, claude lol not what I asked |
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.
RenderingTestCasenow declares its context properties tracked, so the suite stops depending on a legacy read path that RFC 957's end state removes.Groundwork for emberjs/rfcs#1203 ("Deprecate
this.getandthis.seton test contexts"): the framework's own suite is the largest consumer of the pattern that RFC deprecates.Why
Test state reaches templates through the top-level component, and tests mutate it with
set(this.context, ...):That drives a rerender only because a non-tracked property read still entangles a per-property tag.
_getProppays for that on every property read, and removing it is one of the levers measured in #21520. Today the framework's own suite is a large consumer of that path, with 771set(this.context, ...)call sites across 43 files.I have not yet been able to measure how many #21520 failures this accounts for. That branch aborts partway through its own suite, so the two runs I have cover different amounts of it and are not comparable. The claim here is only the one verified below: this is behaviour-neutral on
main.What
this.contextisComponent.extend(attrs), so the fix is one place rather than the 771set(this.context, ...)call sites. Context properties are declared withtracked({ value }), which keeps every existingsetcall reactive under modern semantics. Values that are already decorators (computed properties, injections) bring their own reactivity and pass through untouched.No behaviour change here: 9448 tests, 0 failures.
Not in this PR
runTaskis the other half of the same migration and is harder. 1678 call sites across 84 files assume rendering flushed synchronously:Under the async scheduler it has not flushed. Rewriting those to
awaitturns most of the suite async. The cheaper route is redefiningrunTaskon aflushSync-style escape hatch, which does not exist yet. Worth settling that before touching call sites.🤖 Generated with Claude Code