SDK is defaulting to OnPush strategy

[mkjp2011]

We are finding that our UI components which have animations and effects are not working correctly. We have traced this issue back to the fact the angular SDK is defaulting its change detection as on push. We were able to successfully repo the issue and placed it in a stackblitz project. Attached is a recording as well.

Is there a way to configure this differently? What are some suggestions in addressing the issue? Any issues on changing the strategy that you all can think of?

https://stackblitz-starters-qxz7uv.stackblitz.il)

https://github.com/bloomreach/spa-sdk/assets/56609777/ff15ea57-8a0c-4c4c-9792-aa4b1c3ccd57

[joerideg]

Hi @mkjp2011 , Unfortunately the stackblitz link does not work. Yes the angular sdk page and container components use OnPush because they would only need to update their own state if the page or the container object change. It should not have any effect on the change detection of templates/components projected into their slots, but maybe we are missing something here.

Could you provide some context:

• What SDK version are you using?
• what Content platform are you using, SaaS or PaaS and what version of PaaS?
• Are you rendering CSR or SSR?

If you have an additional stackblitz that is available for us to open that would be a great help

[mkjp2011]

Hi @mkjp2011 , Unfortunately the stackblitz link does not work. Yes the angular sdk page and container components use OnPush because they would only need to update their own state if the page or the container object change. It should not have any effect on the change detection of templates/components projected into their slots, but maybe we are missing something here.

Could you provide some context:

• What SDK version are you using?
• what Content platform are you using, SaaS or PaaS and what version of PaaS?
• Are you rendering CSR or SSR?

If you have an additional stackblitz that is available for us to open that would be a great help

So sorry about that. Here is another stackblitz link that will hopefully with for you

https://stackblitz.com/edit/stackblitz-starters-qxz7uv?file=src%2Fmain.ts

We are using the following:

• version 22.0.0 spa-sdk and ng-sdk
• PaaS - brxm - 14.7.3
• right now we are rendering CSR

[TK11778]

Hi @mkjp2011 , Unfortunately the stackblitz link does not work. Yes the angular sdk page and container components use OnPush because they would only need to update their own state if the page or the container object change. It should not have any effect on the change detection of templates/components projected into their slots, but maybe we are missing something here.

Could you provide some context:

• What SDK version are you using?
• what Content platform are you using, SaaS or PaaS and what version of PaaS?
• Are you rendering CSR or SSR?

If you have an additional stackblitz that is available for us to open that would be a great help

The crux of the issue is that in Angular when a component declares to use the OnPush change detection strategy, all descendants of that component will inherit this and cannot override it. This is by design of Google's Angular team and there is no intention to enable children of a component to bypass and opt out when a parent of it has opted in. Tree shaking will stop when Angular encounters a component using OnPush which has no changes and will not bubble downwards at that point.

Use of OnPush should realistically only be done, if at all, in fairly rare cases where the change detection processes themselves are causing sufficient churn and performance degradation to warrant not shaking the children of a component when said component itself has not changed. There are better avenues to fine tune app performance if this is a legitimate issue, thus the strategy is not default and deemed to be used with caution.

Bottom line here is that the SDK is forcing everything being projected inside of it to use OnPush and not default change detection. This will create severe issues for many more complex components relying on async operations manipulating input bindings.

[TK11778]

FWIW, there are many illustrations of how this works and the potential pitfalls of OnPush with child components. Here is a fairly straightforward writeup for a feature request to Google to allow child components to opt out, but it obviously got squashed as it violates the design pattern they envisioned for it. There are some simple stackblitz examples on it as well that illustrate the lack of change detection in the scenario.

https://github.com/angular/angular/issues/22461

[joerideg]

Hi @mkjp2011 , Thanks for sharing that stackblitz. I took the liberty of creating a brand new generated project, Angular 15, with the SDK 22.0.0 versions. I can reproduce your issues!

The stackblitz for that is here: https://stackblitz.com/edit/github-mwnqyu

I think the issues we identified are:

1. We should not have to add the state output handler for the template to render, this behavior is really odd but I suspect it has to do withe the same underlying cause, using OnPush. The fact that the handler runs in the parent component scope probably triggers the extra change detection check for the component tree to render.
2. Using OnPush appearantly prevents the components to rerender when internal state changes. I was not aware of this, and I think neither was the original author of the SDK. He probably added the OnPush simply as a performance guard.

Also thank you, @TK11778 , for your explanation, and I agree that OnPush should not be necessary, nor is wanted, in the SDK components. I will create an internal ticket to remove this from the SDK components.

I will leave this issue open until a new release has been done to address this. Thanks for the collaboration!

[mkjp2011]

Hi @mkjp2011 , Thanks for sharing that stackblitz. I took the liberty of creating a brand new generated project, Angular 15, with the SDK 22.0.0 versions. I can reproduce your issues!

The stackblitz for that is here: https://stackblitz.com/edit/github-mwnqyu

I think the issues we identified are:

1. We should not have to add the state output handler for the template to render, this behavior is really odd but I suspect it has to do withe the same underlying cause, using OnPush. The fact that the handler runs in the parent component scope probably triggers the extra change detection check for the component tree to render.
2. Using OnPush appearantly prevents the components to rerender when internal state changes. I was not aware of this, and I think neither was the original author of the SDK. He probably added the OnPush simply as a performance guard.

Also thank you, @TK11778 , for your explanation, and I agree that OnPush should not be necessary, nor is wanted, in the SDK components. I will create an internal ticket to remove this from the SDK components.

I will leave this issue open until a new release has been done to address this. Thanks for the collaboration!

Trying to gauge things around this! Appreciate you tremendously on this. How quickly do we think that the new version will be made available?

[joerideg]

I consider it a high prio issue myself and fairly trivial to fix. I cant make any promises, but I aiming to pick it up next sprint. In any case I will let you know as soon as I have an ETA and I will do a prerelease earlier if it makes sense so you can test it and confirm it solves your issue.

[joerideg]

Hey @mkjp2011 I'm sorry for the delay but we fixed it :)

I've updated the stackblitz to use prerelease version (22.0.1-0 under dist-tag spasdk-187). You can see the component now updating as expected without clicking the button, it indeed also fixed the requirement to add (scope) which is no longer necessary.

The internal issue still has to go through our QA process but there should be an official release soon enough. I would appreciate it if you could test the prerelease yourself to double check it resolves your issue.

[joerideg]

Hi @mkjp2011 , a new release 22.0.2 has just been release with the fix. I will close this issue now but feel free to reopen it if you deem it necessary!