fix(core): default the Component decorator to OnPush change detection in JIT - #70261
Closed
ValeraGin wants to merge 1 commit into
Closed
fix(core): default the Component decorator to OnPush change detection in JIT#70261ValeraGin wants to merge 1 commit into
ValeraGin wants to merge 1 commit into
Conversation
… in JIT The AOT compiler already defaults changeDetection to OnPush for components that don't specify a strategy (see compileComponentFromMetadata in packages/compiler/src/render3/view/ compiler.ts, changed in angular#67687). The Component decorator itself, which is what TestBed/Jest exercise when compiling in JIT mode, was never updated and still defaults to ChangeDetectionStrategy.Eager. This means a component with no explicit changeDetection behaves as OnPush in a production AOT build but as Eager under TestBed, so unit tests don't exercise the strategy the app actually ships with. A TODO left by the original OnPush-by-default change anticipated this follow-up ("remove the ts-ignore when OnPush is the default", commit cf3b7fe) but was removed the same day without the value itself being updated. Added a regression test asserting the JIT-compiled definition's onPush flag for a component without an explicit changeDetection.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Author
|
Filed as #70262 with a minimal repro. |
Contributor
|
This is a duplicate of #69898 |
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
3 participants
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.
What's the problem
@Component()compiled in JIT (TestBed, Jest, etc.) still defaultschangeDetectiontoEager. The AOT compiler already defaults toOnPush— that changed in #67687. So a component with nochangeDetectionset behaves asOnPushin a real build but asEagerunder TestBed. Tests don't exercise the same strategy the app ships with.Found this while removing "redundant" explicit
OnPushfrom a bunch of components after upgrading to v22 — one test broke, and only in JIT.Why it's still there
packages/core/src/metadata/directives.tsline 682 was never touched by #67687. It still hardcodes:There's actually a paper trail for this. The very TODO that anticipated the fix:
was removed in
cf3b7fe489, the same day #67687 merged — but only the comment/ts-ignore was removed, not the value it was pointing at. It's beenEagerever since (checkedmainand the latest published22.1.2).The fix
One line:
Eager→OnPushin the decorator default, plus a regression test assertinggetComponentDef(...).onPushfor a component with no explicitchangeDetection.I couldn't run the full internal test suite here (no Bazel setup on my end), so this may well surface spec files elsewhere in the repo that quietly relied on the old JIT default — happy to help track those down if CI turns anything up.