(feat): make fa-icon ChangeDetection.OnPush

damienwebdev commented 3 years ago

Describe the problem you'd like to see solved or task you'd like to see made easier

Make fa-icon ChangeDetection.OnPush

Is this in relation to an existing part of angular-fontawesome or something new?

fa-icon

What is 1 thing that we can do when building this feature that will guarantee that it is awesome?

Add some test cases that use a service to compute the number of times ngAfterViewChecked fires on the fa-icon. After reviewing the component, it doesn't look like this would be a breaking change.

Why would other angular-fontawesome users care about this?

Performance!

On a scale of 1 (sometime in the future) to 10 (absolutely right now), how soon would you recommend we make this feature?

10

Feature request checklist

[x] This is a single feature (i.e. not a re-write of all of Font Awesome)
[x] The title starts with "Feature request: " and is followed by a clear feature name (Ex: Feature request: moar cowbell)
[x] I have searched for existing issues and to the best of my knowledge this is not a duplicate

DaSchTour commented 3 years ago

This should be done with caution. The Icon Component uses tuples so it might be that change detection doesn't work properly if somebody uses a tuple in the host component and changes the values inside without creating a new array.

devoto13 commented 3 years ago

This should be done with caution. The Icon Component uses tuples so it might be that change detection doesn't work properly if somebody uses a tuple in the host component and changes the values inside without creating a new array.

We already use ngOnChanges() hook to re-render the icon, so I don't think that there will be a behavior change in this regard.

On the other hand, I don't expect this change to actually provide any noticeable performance improvement. At the moment fa-icon uses only two trivial Angular bindings, so the change detection run is essentially two === operations per icon. I did a very quick and naive test using the new Angular DevTools extension with and without ChangeDetection.OnPush and the change detection run (without inputs changes) for 500 icons took ~0.1ms in both cases. I might have done something wrong there, but I don't really expect this change to provide any noticeable improvement unless somebody renders an insane amount of icons on the page 😅

damienwebdev commented 3 years ago

Maybe, for a v1, ImmutableFaIconComponent as an experimental component?

I've got a few hundred of such icons on some pages and on the low-cpu devices we test on, a non-trivial amount of time (30ms) is spent just change detecting the leaf icons.

devoto13 commented 3 years ago

@damienwebdev Are you measuring a no-op change detection run or a change detection run, which actually updates the rendered icon? Because ChangeDetection.OnPush can only help with the former, which should already be very fast.

devoto13 commented 2 years ago

I'm going to close it as stale and because I don't quite see how ChangeDetection.OnPush can improve the performance for the fa-icon component, but I'm happy to re-open it if somebody can provide evidence or an example showing otherwise.

FortAwesome / angular-fontawesome