ember.js icon indicating copy to clipboard operation
ember.js copied to clipboard

[Bug] Autotracking issues with EmberArrays

Open njoyard opened this issue 4 years ago β€’ 13 comments

🐞 Describe the Bug

While the docs state that EmberArrays should work with tracking (and they mostly do), autotracking does not work with length or some enumerating methods like every or some.

πŸ”¬ Minimal Reproduction

Here is a minimal reproduction twiddle: https://ember-twiddle.com/6646838412ea59a44bee8bf97defba2f

πŸ˜• Actual Behavior

Clicking on the 'add' button adds an item to the list, but does not update any property defined with a getter. Properties that access length or call some or every on the array do not update.

πŸ€” Expected Behavior

Both counters are updated, and the derived properties are as well

🌍 Environment

  • Ember: 3.18 (in the twiddle), 3.22 (same behaviour locally)
  • Node.js/npm: any
  • OS: any
  • Browser: any

βž• Additional Context

Rewriting the failing code using items.get('length') instead of items.length, and replacing some and every calls with for loops that use length the same way fixes the issue. Or just decorating them with @computed('items.length').

Maybe this is expected, but in this case I would expect the docs to be clearer about what autotracks and what doesn't.

Interestingly, adding @tracked to the items property in the controller fixes it. Which is kind of counter intuitive, because in this case we don't want to track the items property (whose value never changes anyway).

njoyard avatar Oct 29 '20 17:10 njoyard

Another example

I'm seeing a glitch in autotracking.

MyEmberArray.firstObject is tracked MyEmberArray.toArray() is not

so

get max() {
  let values = this.args.data.toArray();
  return Math.max(...values);
}

The above is only called once

get max() {
  console.log(this.args.data.firstObject);
  let values = this.args.data.toArray();
  return Math.max(...values);
}

The above is called every time I pushObject to the array.

Is this by design, or a bug?

BryanCrotaz avatar Nov 02 '20 14:11 BryanCrotaz

Are the Ember arrays in question base Ember arrays (e.g. arrays made with A()) or are they Ember Data arrays, or other implementers of the mutable array mixin?

In either case, these should be tracked, I would consider this a bug. But if they are implementers of the mixin, then it may be an issue on the implementer side.

pzuraq avatar Nov 02 '20 17:11 pzuraq

I've tried with A() and new TrackedArray([]). Both fail to rerun, but adding console.log(myArray.length) to the getter makes it track correctly

BryanCrotaz avatar Nov 02 '20 18:11 BryanCrotaz

new TrackedArray() from tracked-built-ins should not include or work with the toArray method. If it exists on it, it's because Ember extends the Array prototype, but it's not a goal of that library to support it.

pzuraq avatar Nov 02 '20 18:11 pzuraq

This is my code. this.channelNames and this.data are both TrackedArray Every 5 seconds a new value is pushed onto this.data. This getter is not triggered. Adding console.log(this.data.length) in the getter fixes it.

import { extent } from 'd3-array';

@cached
  get yTop(): number {
    let max = this.yMax;
    let margin = 0;
    if (max == 0) {
      let extents = this.channelNames.map(name => extent(this.data, this.yValue(name)));
      // let min = Math.min(...extents.map(e => parseFloat(e[0] ?? '0')));
      max = Math.max(...extents.map(e => parseFloat(e[1] ?? '100')));
      max = Math.max(1, max); // if max is really small, bump it up
      margin = (max/*-min*/)*0.07;
    }
    return max+margin;
  }

BryanCrotaz avatar Nov 02 '20 23:11 BryanCrotaz

@BryanCrotaz if those are TrackedArray instances, then you should open an issue on the tracked-built-ins repo, ideally with a reproduction. tracked-built-ins uses a very different method of autotracking than EmberArray, and Ember really doesn’t have anything to do with it, so its not likely to be an Ember bug.

A full reproduction would really help in either case. How is the cached getter being used, for instance?

pzuraq avatar Nov 03 '20 16:11 pzuraq

Fails the same way with EmberArray.

BryanCrotaz avatar Nov 03 '20 16:11 BryanCrotaz

I'd like to investigate. Where in the source is the array tracking code?

BryanCrotaz avatar Nov 03 '20 16:11 BryanCrotaz

Two things can fail the same way and have different root causes.

I have tests for TrackedArray.map reactivity here: https://github.com/pzuraq/tracked-built-ins/blob/master/tests/unit/array-test.js#L408-L421

The implementation of TrackedArray and its tracking is all in this file: https://github.com/pzuraq/tracked-built-ins/blob/master/addon/-private/array.ts

For EmberArray, it's a bit different, it has to go through @tracked property access due to backwards compatibility constraints. As such, it's a bit diffused throughout ember-source.

  • Entanglement happens here for tracked properties, here for computeds and get() calls.
  • Dirtying happens here for pushObject in native arrays. push is not instrumented. In non native arrays (e.g. Ember Data arrays, other implementers) this could not be called correctly, which could be the source of the issue.

pzuraq avatar Nov 03 '20 16:11 pzuraq

As it works if you read array.length, I assume it's entanglement that's failing...

BryanCrotaz avatar Nov 03 '20 16:11 BryanCrotaz

Sure, that makes sense. Given these are two separate array implementations, I would also guess that they are failing for different reasons. Since we have tests for TrackedArray, I’m going to need more for a reproduction in that case, and since the usage of EmberArray with tracking is more complicated in general, I could also use more info for a reproduction in that case.

I definitely believe there are failures occurring here, this stuff is just complicated and it helps if we have more thorough reproductions that demonstrate the problem, and what we missed in our tests! πŸ˜„

pzuraq avatar Nov 03 '20 17:11 pzuraq

I'll fork and create some failing test cases

BryanCrotaz avatar Nov 03 '20 17:11 BryanCrotaz

also hitting this in some cases. another workaround that works for me is to do this.data['[]'].map(...)

patricklx avatar Jun 13 '22 08:06 patricklx