[clrDgSelectable]="false" in clr-grid does not disable select in v-4

[edgas316]

Describe the bug

using [clrDgSelectable]="false" in clr-grid does not disable select in v-4

How to reproduce

https://stackblitz.com/edit/clarity-v4-light-theme-k7dk5i?file=src%2Fapp%2Fapp.component.html

Versions

App

• Angular: angular 10
• Node: [e.g. 8.10.0]
• Clarity: 4.0

Device:

• Type: any device
• OS: any oc
• Browser any
• Version [e.g. 22]

Additional notes

Add any other notes about the problem here.

[bdryanovski]

There is a work-around for this;

If you change the order of the arguments everything will work:

// Before
<clr-dg-row [clrDgSelectable]="state" *clrDgItems="let tenant of tenants" [clrDgItem]="tenant">

// After
<clr-dg-row [clrDgItem]="tenant" [clrDgSelectable]="state" *clrDgItems="let tenant of tenants">

The issue is that we are doing some calculations inside the setter that we don't need to do there and need to move it outside in something like ngOnChanges.

[gnomeontherun]

@Shijir can you share a bit about the order problem, I remember you had discovered changes in a previous Angular build but the details elude me.

[bdryanovski]

I could share some of my findings.

The issue is how and when Angular decide to assign values from @Inputs. In the previous version, this was an undocumented but working solution - the order of there definition inside the @Component. Now (after Ivy), this is not the case, there is no guarantee that the input that you want to use in another input will be assigned.

There are good examples here

So the best and correct way of doing it - is to not depend on other inputs values inside getter & setters. And use Angular lifecycle (ngOnChanges) to handle it.

The bigger problem is that is hard to say where else this had been done - we have a lot of getter & setters and there is no good way to write a test about it. In the test runner, everything will be fine cause there are at most of the time super small delays, or the data is already passed to the component so no lifecycle is trigger. And also the test setup is doing some magic under the hood so we could not be sure of it. I want to say here that no test before that had been failing so no way to detect it. The only way to start notice them that I found is randomly to move the inputs inside the templates - but this is really hard to be done inside a test (not impossible but hard).

The best thing that we could do is to keep an eye for this type of pattern.

@Input('name') name: string;

@Input('title') 
public set title(n: string) {
  this.fullname = `${n} ${this.name}`;  /* this.name may be undefined at that point */
}

Better will be to use the lifecycle to assign it.

onChanges(changes: SimpleChanges) {
  if (
      /* handle only the inputs that we care about */
      changes.title 
      /* make sure that we won't run the code too many times */
      && changes.title.previousValue !== changes.title.currentValue
      ) {
         /* handle changes goes here */
      }
}

There is not only the use case when there is the use of another input inside an input (inception). Some getter or setter could call a public/private method that uses inside another this.someInput as part of the function.

@Input('name') string;

@Input('age') 
public set age(number: number) {
  this.calculateSomething(number)
}

private calculateSomething(age) {
  if (this.name === 'papa') {  /* this.name could be `undefined` */
     return true;
  }
  return age > 21; 
}

This is the third time that we have an issue related to this.

This is the issue that @gnomeontherun mention above

[gnomeontherun]

@Shijir can you please share about the order problem too from your experience?

[Shijir]

So before Ivy, no matter in what order users provide their input values, their values used to be assigned in the order their corresponding @input properties are specified in the component class. But that was an “unintentional" feature from Angular. So when it comes to a @input property that’s dependent on another @input property, we have to coordinate them through their getters/setters, which could be better than using ngOnChange as it specifically targets the property itself. I have encountered and fixed a few such cases in datagrid-column.ts when we were migrating to Angular 9 and Ivy.

So in the example of datagrid-row.ts, we could do something like this:

private _item: T;

@Input('clrDgItem') set item(value: T) {
  this._item = value;
  if(this._selectable) {
    this.clrDgSelectable = this._selectable;
  }
}

private _selectable: boolean;

@Input('clrDgSelectable')
public set clrDgSelectable(value: boolean) {
  if(this._item) {
    this.selection.lockItem(this._item, value === false);
  } else {
  // attempted value during the absence of this._item
  this._selectable = value;
  }
}

[Jinnie]

which could be better than using ngOnChange as it specifically targets the property itself

Can you explain in some more detail why do you prefer it to ngOnChange, please?
From my perspective, while getters worked as they did pre-Ivy, it was great to use them. But now that there's no guarantee of their order, we'd better use the next logical framework provided bus stop. The checks and calls between the getters will do the work, of course, but I find them harder to understand and maintain and it looks to me as a framework-patching pattern, which I personally feel a bit suspicious about.

[gnomeontherun]

@bdryanovski can you confirm that your changes addressed this bug and close it?

[rbirkgit]

This is still an issue with v4.0.14

Workaround of putting [clrDgSelectable] as the last parameter does work.

[github-actions]

Hi there 👋, this is an automated message. To help Clarity keep track of discussions, we automatically lock closed issues after 14 days. Please look for another open issue or open a new issue with updated details and reference this one as necessary.