angular-fontawesome
angular-fontawesome copied to clipboard
FortAwesome
(feat): make fa-icon ChangeDetection.OnPush
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?
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
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.
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 😅
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.
@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.
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.