Don't completely agree with dont-use-property-bindings-to-pass-static-strings rule

[alexzuza]

Consider the following components structure:

@Component({
  selector: 'child',
  template: 'child'
})
export class Child {
  @Input() textProp: string;

  @Input() propForCheck: string;
}

@Component({
  selector: 'my-app',
  template: `
    <child textProp="someText" [propForCheck]="'checkMe'"></child>
  `
})
export class AppComponent {}

I distinguish two implementations for such cases:

ViewEngine (Angular 4-7...)

This renderer will generate ngfactory that will looks like:

function View_AppComponent_0(_l) {
        return jit_viewDef_1(0, [(_l()(),
        jit_elementDef_2(0, 0, null, null, 1, 'child', [['textProp', 'someText']], null, null, null, jit_View_Child_0_3, jit__object_Object__4)), jit_directiveDef_5(1, 49152, null, 0, jit_Child_6, [], {
            textProp: [0, 'textProp'],
            propForCheck: [1, 'propForCheck']
        }, null)], function(_ck, _v) {
            var currVal_0 = 'someText';
            var currVal_1 = 'checkMe';
            _ck(_v, 1, 0, currVal_0, currVal_1);
        }, null);
    }

where updateDirectives part is:

function(_ck, _v) {
   var currVal_0 = 'someText';
   var currVal_1 = 'checkMe';
   _ck(_v, 1, 0, currVal_0, currVal_1);
},

As we can see above Angular will still check our static text property regardless we didn't use property binding explicity. So we can conclude that ViewEngine still treats "static attribute binding" as property binding that needs to be checked.

In addition, if we take a look at elements panel:

<child textprop="someText" ng-reflect-text-prop="someText"
               ng-reflect-prop-for-check="checkMe">child</child>

we can also notice textprop attribute.

In summary, I would say that using textProp="someText" instead of [textProp]="'someText" will not give us better performance since it's treated the same as property binding and also blows our html by adding attribute on element through.

Ivy(incoming new renderer)

In Ivy we will have ngDefinition with compiled template like:

var _c0 = ['textProp', 'someText', 1, 'propForCheck'];
...
template: function AppComponent_Template(rf, ctx) {
    if (rf & 1) { // insert mode
      jit_element_2(0, 'child', _c0);
    }
    if (rf & 2) { // update mode
      jit_elementProperty_3(0, 'propForCheck', jit_bind_4('checkMe'));
    }
},

Here we can apply that rule since our static property treats as attribute and won't be checked during CD cycle

[d3lm]

Wow, very detailed and in-depth elaboration @alexzuza! Thanks so much for opening this issue and discussing this with us.

I totally agree with you on @Input properties and it's correct that the ViewEngine will still check a "static attribute binding". But this is not the case for native attributes on an element, such as class or id.

If we stick to your example and I use a property binding on the id then this will result in a function that is executed to check the binding.

So our code is:

@Component({
  selector: 'my-app',
  template: `
    <child [id]="'foo'" textProp="someText"></child>
  `
})
export class AppComponent {}

This will result in the following ngFactory for the AppComponent:

function View_AppComponent_0(_l) {
  return _angular_core__WEBPACK_IMPORTED_MODULE_1__['ɵvid'](
      ...,
      function(_ck, _v) {
        var currVal_1 = 'new text';
        _ck(_v, 1, 0, currVal_1);
      },
      function(_ck, _v) {
        var currVal_0 = 'foo';
        _ck(_v, 0, 0, currVal_0);
      }
    )

We can see that there is now a function generated to check the property binding on the id, which is the last one. If I convert this into a "static attribute binding" like this:

@Component({
  selector: 'my-app',
  template: `
    <child id="foo" textProp="someText"></child>
  `
})
export class AppComponent {}

We end up will the following ngFactory:

function View_AppComponent_0(_l) {
  return _angular_core__WEBPACK_IMPORTED_MODULE_1__['ɵvid'](
      ...,
      function(_ck, _v) {
        var currVal_1 = 'new text';
        _ck(_v, 1, 0, currVal_1);
      },
      null
    )

So there is no longer a function for the id property binding. To me, if you sprinkle this across your entire application, can impact the performance. You have all these unnecessary binding checks for static values, here just strings.

[alexzuza]

@d3lm

Thanks for the response! I like this project.

The rule states:

If we have static strings that we want to pass to a native HTML element or a component

Maybe that confused me.

If we want to get those static native element attributes(id, class, title etc) in component we have to use either @Input or @Attribute decorators.

And in current ViewEngine:

• using @Input doesn't save us from the check(in this case attribute becames property binding no matter how it was defined: title="text" or [title]="'text'" or title="{{text}}" or even data-title="text")

• injecting that property with @Attribute decorator seems to do trick(https://netbasal.com/getting-to-know-the-attribute-decorator-in-angular-4f7c9fb61243)

And of course we can read those attributes from ElementRef injected in component

[d3lm]

Yes you are completely right. Maybe I we need to reword the item a little bit. What I meant was basically using property bindings on native element attributes such as id, class, title etc.

I have seen many projects where developers used property bindings on these attributes with static strings but they didn't want those to be passed down to a component. They simply wanted to set the value. When bringing that up in the code review they often responded with "I thought I had to use property bindings everywhere". Which is not correct and if miss used, it can actually impact the performance in large applications if there are dozens of "static" property bindings for which there are no Inputs that need to be checked during CD.

Maybe you have an idea how to properly reword this item? I'd love to hear your thoughts on this.

[KwintenP]

Hi Alex!

Thanks a lot for your detailed input and description! You are 100% correct that this is very ambiguous. Would you be ok in rewording it from:

don't use property bindings to pass static strings

to

don't use property bindings to pass static strings to native attributes

Or something similar. (and also updating the content of the item of course). Do you think this makes more sense?

[d3lm]

I like your suggestion @KwintenP.

[alexzuza]

@d3lm I'm not good at this but the @KwintenP's suggestion sounds better

[KwintenP]

Perfect, I'll try to update this item asap and ask you to review when I'm finished. Thanks again for the input!

[KwintenP]

27 Opened a PR for this.

@alexzuza @d3lm feel free to comment ;)

[d3lm]

Approved! Does the change make sense to you @alexzuza? Is it more clear now?

[alexzuza]

@d3lm @KwintenP Big thanks for looking at this. I think I can close the issue.

[KwintenP]

Thank you a bunch for your input Alex!