Feature request: Enforce value of @Component.changeDetection

[philipbulley]

In the project I'm currently working on, it'd be useful to ensure that all components are set to use changeDetection: ChangeDetectionStrategy.OnPush.

[mgechev]

Sounds good, I'm only afraid it could be misleading for components which produce/rely on side-effects.

I'm not sure there's a good way to predict if a component is suitable for OnPush ChangeDetectionStrategy. Do you have any ideas?

[mgechev]

Let's close it since it's very hard to guess if a component produces a side-effect &/|| depends on global state or not.

[guojenman]

@mgechev How about enforcing the existence of changeDetection? At least this way, it's a conscious decision by the developer to say whether it is or not. This would prevent accidental default change detection by an accidental omission.

[ghost]

@mgechev A rule for this is landing soon in material2, would be nice to include this rule in codelyzer. It uses a whitelist to disable it for certain components.
angular/components#5707

[mgechev]

Okay, lets reopen the issue.

[ghost]

An even more beautiful rule has landed in material2, to lint any property value of any decorator:
https://github.com/angular/material2/blob/master/tools/tslint-rules/validateDecoratorsRule.ts

[dominique-mueller]

In our case, we're developing an Angular component library in which every component must have set the change detection strategy to OnPush. We also try to build our applications using OnPush everywhere.

Thus, it would be awesome to have linting rule for that!

[mgechev]

Okay, lets do that. Maybe as argument of the rule we can provide a regex for matching the components which should have OnPush. By default it can be .*.

[amcdnl]

Whats the status on this? Would love to incorporate this.

[wKoza]

I think that you can do it. If we want to bypass this rule, we can also simply mark our component with tslint:disable:onPushStrategy(with a comment)

[mgechev]

Following the feature request process that lodash has, I'm closing the issue and adding votes needed label.

Later we can prioritize the feature requests by popularity and include them in a future release.

[buu700]

I'm not sure there's a good way to predict if a component is suitable for OnPush ChangeDetectionStrategy. Do you have any ideas?

What about validating that every variable referenced in the template is either @Input() or readonly (with the latter intended to cover static values and observables)? (There's also the issue of getters — that feels hairier to properly address, but maybe just validate that they don't blatantly reference non-read-only class members?)

There would probably be some exceptions where users need to disable the rule for certain components, like when relying on manual change detection initiation, but unless I'm forgetting something I think just about any ideal/idiomatic OnPush-compatible code would comply with the above.

[mrmeku]

+1 vote. If and when you decide to go ahead with this @mgechev I'd be willing to try and implement it. I think detecting whether something should be OnPush is nearly impossible as you've said, but requiring that you add your component to a whitelist of things that are exempt is super easy. That's what I'm voting for

[dominique-mueller]

Assuming there is no relevant difference between defining a whitelist array in the tslint file vs. using tslint:disable comments, I would certainly prefer the comment-based solution.

In my opinion, tslint:disable comments are much easier to maintain because the linting exception is closer to the actual source code it affects.

[mrmeku]

I would too. I used the term white list intentionally ambiguous just in case other people felt differently.

…

On Mon, Jul 23, 2018, 12:22 PM Dominique Müller ***@***.***> wrote: Assuming there is no relevant difference between defining a whitelist array in the tslint file vs. using tslint:disable comments, I would certainly prefer the comment-based solution. In my opinion, tslint:disable comments are much easier to maintain because the linting exception is closer to the actual source code it affects. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#135 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHteO1rYZCEXx1xGSO8IbYty2ysRv_sUks5uJhR4gaJpZM4KpeID> .

[mgechev]

@dominique-mueller I agree with you. @mrmeku feel free to take this! If you do, just let me know so I can assign you to the issue.

[mrmeku]

I'll take it! Assign it to me :D

[mgechev]

@mrmeku I'll assign it to myself so that nobody takes it. GitHub does not allow me to attach the issue to you.

[ajantsch]

We're working on optimizing on of our apps right now, and one of the measures we take is to make all the components to use ChangeDetectionStrategy.onPush, while during the transition period we would also like to whitelist some components that haven't been updated yet. So this feature would be very beneficial for us 👍

[lazarljubenovic]

Why don't you just the use the comments to disable linting? Whitelisting in a separate file is cumbersome, you have info about the class scattered around.

// why like this?
whitelist: ["FooCmp"]

class FooCmp {}

// when you can do this!
// whitelist
class FooCmp {}

The exact same rationale is in Angular's own styleguide for using inputs: [] vs @Input.

// why like this?
inputs: ['foo']

public foo: string

// when you can do this!
@Input()
public foo: string

---

Imagine if tslint config had a whitelist for lines you want to disable it in:

// why like this?
"max-line-length": [true, 140, {"whitelist": ["path/to/file#L20", "path/to/other/file#L30"]}]

// path/to/file
20 | (super long line)

// path/to/other/file
30 | (super long line)

// when you can do this!
// path/to/file
19 | // tslint:disable-next-line:max-line-length
20 | (super long line)

// path/to/other/file
29 | // tslint:disable-next-line:max-line-length
30 | (super long line)

It's always better to define things about X near X instead of a config file which references X.

[keradus]

I believe that @ajantsch is supper fine with disabling-by-comment approach,
he simply gave background why he is so eager to see the feature ;)
I wouldn't imagine changing all components at once, also - thus transition period idea he described.

btw, I would be also supper happy to see this live ! 👍
It looks like one with most votes also ( https://github.com/mgechev/codelyzer/issues?utf8=%E2%9C%93&q=label%3A%22votes+needed%22+sort%3Areactions-%2B1-desc+ )

[mgechev]

Anyone wants to take this issue? I'll be happy to assist if needed.