Switch v0.8 Angular Renderer to use OnPush change detection#877
Switch v0.8 Angular Renderer to use OnPush change detection#877gspencergoog wants to merge 5 commits intogoogle:mainfrom
OnPush change detection#877Conversation
gspencergoog
changed the title
OnPush change detection
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring, migrating the v0.8 Angular renderer components to use ChangeDetectionStrategy.OnPush. This is an excellent move for performance and determinism. The changes are well-executed, and the addition of comprehensive unit tests for the components is a great enhancement. My review focuses on a few areas to further improve code quality: I've identified a potential bug in a catalog binding, and have suggestions to improve type safety and the robustness of some of the new tests.
| inputBinding('text', () => properties.text ?? null), | ||
| inputBinding('label', () => properties.label), | ||
| inputBinding('textFieldType', () => properties.textFieldType), | ||
| inputBinding('inputType', () => properties.textFieldType), |
There was a problem hiding this comment.
There appears to be a mismatch in the TextField catalog binding. The binding is configured for an input named inputType, but the TextField component's input property is named textFieldType. This will prevent the textFieldType property from being correctly passed to the component, likely causing it to default to 'text' regardless of the model's value.
| inputBinding('text', () => properties.text ?? null), | |
| inputBinding('label', () => properties.label), | |
| inputBinding('textFieldType', () => properties.textFieldType), | |
| inputBinding('inputType', () => properties.textFieldType), | |
| inputBinding('text', () => properties.text ?? null), | |
| inputBinding('label', () => properties.label), | |
| inputBinding('textFieldType', () => properties.textFieldType), |
| fixture.detectChanges(); | ||
| await fixture.whenStable(); | ||
| fixture.detectChanges(); | ||
| await fixture.whenStable(); | ||
| fixture.detectChanges(); |
There was a problem hiding this comment.
This sequence of detectChanges and whenStable calls, while likely correct for handling nested async rendering, is a bit difficult to read and could be brittle. To improve test clarity and maintainability, I recommend extracting this stabilization logic into a shared helper function. This would make the test's intent clearer and allow for easier reuse across other tests that might face similar async scenarios.
|
|
||
| if (this.areHintedStyles(styles)) { | ||
| additionalStyles = styles[usageHint ?? 'body']; | ||
| additionalStyles = (styles as any)[usageHint ?? 'body']; |
There was a problem hiding this comment.
The use of as any here weakens type safety. The areHintedStyles type guard is intended to prevent this, but there's a small inconsistency: it checks for an h6 key, which is missing from the HintedStyles interface definition.
To improve this, I recommend two changes:
- Add
h6: Record<string, string>;to theHintedStylesinterface to align it with the type guard. - Replace the
as anycast with a more type-safe approach, like(styles as HintedStyles)[(usageHint ?? 'body') as keyof HintedStyles];.
| fixture.detectChanges(); | ||
|
|
||
| // Wait for async render | ||
| await new Promise((resolve) => setTimeout(resolve, 10)); |
There was a problem hiding this comment.
Using setTimeout in unit tests can introduce flakiness and make them less deterministic. Angular's testing utilities provide more robust ways to handle asynchronous operations. I recommend replacing await new Promise((resolve) => setTimeout(resolve, 10)); with await fixture.whenStable(); to wait for promises like the dynamic component import to resolve.
jacobsimionato
left a comment
jacobsimionato
left a comment
There was a problem hiding this comment.
I saw @ditman did something similar and abandoned it in #805 , and @crisbeto added EAGER explicitly in #726 to prevent a breakage - not sure if it was a theoretical breakage or a real breakage. Maybe we should consult with one of them before submitting, in case there are unintended consequences in google's monorepo? Does this block submitting v0.9 code, or it's a nice to have for consistency?
It is orthogonal to 0.9, although 0.9 is set up to use OnPush. It doesn't block anything. I agree we should get more eyes on (once I fix the build issues). |
|
We abandoned my prior attempt because the testing story is not good, and we're not sure of what can break by blanket-applying this to the v0.8 codebase. The deal was to apply these as bugs were found on a per-component basis. I think it's okay to do v0.9 fully based on OnPush, but leave 0.8 as is, and bring in fixes as we find them via bug reports (I suppose those are going to be mostly internal bugs, if any) (/cc @jgindin) |
gspencergoog
changed the title
OnPush change detectionOnPush change detection
Okay, I'll close this for now, then, and abandon the change until we see reasons to change it. |
|
Oh, wait, there are a lot of new tests here. I think I'll just revert the |
|
Closing in favor of #882 |
3 participants
Description
This PR refactors all components in the v0.8 Angular renderer from
ChangeDetectionStrategy.EagertoChangeDetectionStrategy.OnPushfor improved rendering performance and determinism.Changes
renderers/angular/v0_8/components/: Updated components to explicitly consume@Inputvariables and bind withOnPushchange detection.renderers/angular/v0_8/rendering/renderer.ts: Replaced signaleffectsetups with@Input()variables andngOnChangessupport to satisfy standard reactive input triggering.renderers/angular/v0_8/rendering/renderer.spec.ts: Updated fixturetick(10)triggers inside async assertions becauseOnPushrequires explicit detection cycles in tests.Impact & Risks
Testing
renderers/angular/v0_8unit tests successfully pass with continuous integration builds.