visibleColumns does not refresh with ember source >= 3.23

[cah-danmonroe]

As of ember 3.23, @filterBy('columns', 'visible', true) visibleColumns; does not get updated and always returns 0.

After some investigation, we found that the order in which code executes is different between 3.22 and 3.23.

With 3.22, registerColumn gets called for each column prior to the call to visibleColumns. With 3.23, it is reversed and visibleColumns never gets called again.

I added some console logs to show the difference:

3.22:

3.23:

We added a "fix" that results in the expected behavior but not sure if this is the way you would want to resolve the issue. We added a notifyPropertyChange to registerColumn and the visibleColumns updates as expected.

`registerColumn(column) {

if (this.isColumnVisible) {

  let visible = this.isColumnVisible(column);

  set(column, 'visible', visible);

}

let columns = this.get('columns');

if (!columns.includes(column)) {

  columns.push(column);

  once(() => this.notifyPropertyChange('visibleColumns'));  // added

}

}`

I can do a PR for this or we can resolve it in a different way.

[cah-brian-gantzler]

The root cause is that visible columns is not an ember array and is using push instead of push object, thus computed doesnt see the change after being called first. This is because the expected behaviour is that the columns array would be fully built before anything else inspected it.

The registerColumn code is in init(), perhaps moving it to constructor() will get it built earlier in the process.

making columns an ember array and using pushObject is a possibility

If @tracked is an option, changing things from computed to @tracked would also be a way to go. (I would vote for this one)

[miguelcobain]

Using @tracked on arrays doesn't make the push calls "observable", I think. In other words, @tracked is not a replacement for ember arrays.

Do tests fail on this Ember version? I suppose so. If they don't, then it would be good to have a failing test.

I think that, if making columns an EmberArray solves the problem, that is the correct course of action.

We should start thinking about migrating this addon to glimmer components, and at that point @tracked will become our "bread and butter".

[cah-brian-gantzler]

Using @tracked you would make the array immutable and just assign a new array (there arent that many columns to worry about performance) because yes the push would not trigger. The @tracked from here https://github.com/pzuraq/tracked-built-ins will track the array push, but uses proxies and is not as backward compatible as just doing an immutable array.

Yes, one test fails on ember 3.23+ The last time tests ran on your CI 3.23 was not out I think so it didnt fail on the release channel, but Im surprised the canary channel didnt trigger the fail. Any PR that would run at the moment should fail on the release channel here https://github.com/miguelcobain/ember-yeti-table/blob/master/tests/integration/components/yeti-table/general-test.js#L721

The way Dan fixed it was one line low impact, so might be an ok fix for the moment. Changing it to an ember array would take some more testing to see that it did not impact somewhere else, so effort might be better spent fixing when converting to glimmer

[miguelcobain]

@cah-briangantzler @cah-danmonroe on a second thought, the "one line low impact" fix might be the best at this point.

[cah-danmonroe]

https://github.com/miguelcobain/ember-yeti-table/pull/340

[cah-danmonroe]

Thanks!

[miguelcobain]

I updated the whole project and moved from travis to gh actions. We should have an easier time now. 😅 Will release this as 1.6.2 shortly.

Thank you!