Refactor: app config and header component signals

[chrismclarke]

PR Checklist

• PR title descriptive (can be used in release notes)

TODO

• Merge after Refactor: app config runtime #2423

Description

Following recent improvements to app config handling, update the main app.component.ts and app.header.ts to make use of signals, computes and effects. These should be generally more performant and simplify some of the more complex usage patterns (e.g. rxjs observable subscriptions)

Review Notes

See demo below triggering skin change which should also trigger knock-ons to theme, header and footer config. These appeared to all be working as expected when testing locally, but would be good to verify

Screenity.video.-.Sep.18.2024.webm

Dev Notes

I've started just refactoring these components as they are the most visible in the app and so should have the most immediate benefit/impact. There's around 10-15 additional services which call the appConfigService and could potentially be refactored, although most of these are less related to UI (but would still likely benefit from more fine-grained update handling).

It's hard to measure the exact performance improvements, although see benchmarks from angular profiler below. These are skewed however as angular change detection runs more greedily in development mode than production anyways, but should still give some indication

Git Issues

Closes #

Screenshots/Videos

Before
Using angular devtools profiler, can see while switching skins a huge number of change detection cycles triggered

After
Minimal detection cycles triggered

[chrismclarke]

@jfmcquade
Apologies this PR was left in draft waiting for #2423 but I forgot to mark as ready for review. I think it should be now, hopefully it should work alongside other changes you're planning on implementing for header/footer changes.

[jfmcquade]

Thanks, @chrismclarke. Nice improvements to have ahead of implementing template-level header and footer customisation.

Everything seem to be working as expected from my functional testing. And nice to see evidence of the improved change detection handling, even if the actual performance gains are hard to quantify.

[jfmcquade]

This pattern makes sense to me, and we've already introduced similar effects in other components. But I note that the Angular docs discourage using effects in general, and further suggest that allowSignalWrites can be enabled if "absolutely necessary".

Do you have an idea of what a more Angular-y structure would be? It seems obvious to me that effects are useful and that we might want to set signals inside them

[chrismclarke]

My rough take on it is given how easy it is to accidentally end up in infinite loops it makes sense to be cautious about using allowSignalWrites, and intentional when using to set signals. Pretty much any time you use there could be an alternate pattern available using computed signals instead (usually with additional intermediate variables), or by using regular variables and cdr methods to prompt change detection.

I.e. in this example the signal write is so that the marginTop can be set following changes and automatically detected. If we just had a regular marginTop variable that is set as part of the effect (instead of a signal) we could update and then call cdr.markForCheck. I don't like this pattern as much as using both signals and cdr feels a bit wrong given they're meant to address the same problem

Alternatively you could make both headerHeight and headerCollapse computed properties that depend on headerConfig, and marginTop a third computed property that depends on the other two. Again, I'm not a huge fan of this pattern - what you save in reactivity chain you lose by having two separate computed functions that duplicate some amount of code between and lose visibility on the relation between them. But all this is quite subjective, and I can see why angular struggles to give clear guidance of when/when not to use