perf(timepicker): replaced ngClass method calls with class fields

[ekulabuhov]

Binding to a method makes Angular call it every time a change detection happens. Depending on complexity of your app it could be quite often. Using string field is a lot cheaper without sacrificing on functionality.

Before submitting a pull request, please make sure you have at least performed the following:

• read and followed the CONTRIBUTING.md guide.
• built and tested the changes locally.
• added/updated any applicable tests.
• added/updated any applicable API documentation.
• added/updated any applicable demos.

[pkozlowski-opensource]

@ekulabuhov thnx for the PR

What you are saving here is really nothing compared to all the complex processing that ngClass needs to do... If you really want to improve perf here a way of doing so would be to get rid of ngClass usage. If you can prepare such PR we would merge it, otherwise I don't think that gain is measurable (and has a drawback of adding more public properties.

[ekulabuhov]

@pkozlowski-opensource thanks for putting your time to review this.

This PR stems from our very own specific use case, where in combination with other components this led to >20 sec screen freezes.

Could you elaborate please on how would you proceed to remove the ngClass from the code?

I'm also not sure what you mean by public properties as for components public API is denoted by @Input and @Output?

[pkozlowski-opensource]

This PR stems from our very own specific use case, where in combination with other components this led to >20 sec screen freezes.

@ekulabuhov are you sure that 20+ sec JS processing was coming from <ngb-timepicker>? If so I would like to see chrome timeline recording and investigate it deeper.

In any case if there is anything to improve here is not the function call but rather usage of ngClass. You could simply replace [ngClass]="buttonSizeClass" with [class.btn-sm]="isSmallSize() [class.btn-lg]="isLargeSize() where isSmallSize is defined as:

isSmallSize() {
  return this.size === 'small';
}

isLargeSize() and handling of form-control-sm/form-control-lg would be analogous.

Would you be willing to update your PR with the described changes?

[benouat]

I would even go further and transform them into ES getters

get isSmallSize() {
  return this.size === 'small';
}

[class.btn-sm]="isSmallSize"

(personally & visually speaking I don't like () in templates except for events)

[ekulabuhov]

The problem was with the third party library that was using Mutation Observer API on the full DOM. It seems like whenever Angular was running the change detection cycle it was briefly modifying the class attribute, then setting it back to the same value. That was triggering the Mutation Observer. Change detection was running every frame.

I will need to check if the proposed solution is better because I believe that binding to functions like that still triggers mutation observer.

What's the reason for avoiding the use of ngOnChange? Am I wrong in thinking that it's better suited here?

[pkozlowski-opensource]

It seems like whenever Angular was running the change detection cycle it was briefly modifying the class attribute, then setting it back to the same value.

@ekulabuhov - this would be a bug in Angular - or more precisely in the ngClass directive. If you can reproduce it I could have a look (I've implemented ngClass on the Angular side).

I will need to check if the proposed solution is better because I believe that binding to functions like that still triggers mutation observer.

Binding that don't change value should not trigger any DOM manipulation. If it does it is a bug in Angular that we would have to fix (but we need to have a reproduce scenario first). Again, if anything behaves funky it would be [ngClass].

What's the reason for avoiding the use of ngOnChange? Am I wrong in thinking that it's better suited here?

It just adds more code and intermediate state that needs to be a public member of a given class. More code and state makes things harder to reason about.

To sum up: simple functions in bindings should not be a problem and should not trigger additional DOM manipulation. If they do it would be a bug in Angular that needs fixing. From the description I would assume that the bug is in ngClass, if there is any bug here - this is why need a reproduce scenario to confirm.

In any case I believe that implementing proposed refactoring would solve your issue.

[ekulabuhov]

@ekulabuhov - this would be a bug in Angular - or more precisely in the ngClass directive. If you can reproduce it I could have a look (I've implemented ngClass on the Angular side).

Oh, that would be awesome if you could take a look! I have reproduced this issue here: https://stackblitz.com/edit/logrocket-bug

I recommend Opening it in a New Window. Then open dev-tools. Then go to src/app/app.component.ts and comment out line 36 (observer.disconnect()). For me, it hangs the tab with infinite console.logs().

[pkozlowski-opensource]

@ekulabuhov as hinted before this is a corner case / bug in the ngClass implementation, see angular/angular#25518

I've sent a PR to work-around it on the timepicker side, see: #2617

I'm going to close your PR as it is better to get rid of ngClass altogether. Please follow #2617 for the resolution on our end.

[ekulabuhov]

Thanks a lot for getting to the bottom of this problem! I've subscribed to 2617.