[3.16] Bug with set and array

[boris-petrov]

The following code:

const x = A([1]);
x.addObserver('0', function() {});
x.removeAt(0);
x.pushObject(2);
console.log(x[0]);
x.set(0, 3);
console.log(x[0]);

Prints two times 2. The same is true when using Ember.set instead of the set method on the array.

The funny thing is that removing either the removeAt line or the addObserver line fixes the problem.

Any ideas for a workaround would be welcome as this is a problem for us.

P.S. Calling x.replace(0, 1, [3]); instead of x.set(0, 3); seems to work fine. However, this doesn't work in my case because I have a custom implementation of EmberArray and I've overwritten replace to behave differently (that's why I'm calling Ember.set internally and noticed that bug). How can I call the "original" replace? I tried this._super and a few other things as well but to no avail.

[pzuraq]

The issue is that the mandatory setter code is not distinguishing between 0 and "0". The observer adds a mandatory setter for "0", but does not add an appropriate getter for it to the array. Try this.set('0', 3) and see if that works.

FWIW, you probably shouldn't be using this.set() in your custom array, I would create a separate internal array representation instead. This will be much more performant, since you aren't messing with the shape of the array constantly (which this will be doing).

Also, you can implement a custom "array" by implementing the following methods/properties on any class:

• objectAt
• replace
• get length
• forEach

It should work in general in the system if it has all of those, and doesn't need to extend from EmberArray. Trying to formalize that more in the near future.

Edit: Also notify the [] property whenever something changes, and entangle it in objectAt

[pzuraq]

Ah, interesting. So when you remove the item from the array, it also splices out its property descriptor, which is why there's a mismatch with mandatory getters/setters here.

I don't think mandatory setters really make sense in this case. Will add a PR to remove.

[boris-petrov]

@pzuraq - thanks for the detailed explanations as always!

this.set('0', 3) unfortunately didn't help. Using a string instead of an int everywhere doesn't fix the issue.

As for the internal array representation - I'm not sure why that would be more performant... what I have exactly is as follows:

const properties: ThisType<ISmartMutableArrayProxy<T>> = {
  objectAt(index: number): T | undefined {
    ...
  },

  unknownProperty(key: string | number): T | undefined {
    ...
  },

  firstObject: computed('0', {
    get(this: ISmartMutableArrayProxy<T>): T {
      ...
    },
    set(this: ISmartMutableArrayProxy<T>, _, value: any): T {
      ...
    },
  }),

  lastObject: computed('length', {
    get(this: ISmartMutableArrayProxy<T>): T {
      ...
    },
    set(this: ISmartMutableArrayProxy<T>, _, value): T {
      ...
    },
  }),

  setUnknownProperty(key: string | number, value: any): T {
    ...
  },

  replace(index: number, amount: number, objects = []): ISmartMutableArrayProxy<T> {
    ...
  },

  includes(object: any, startAt?: number): boolean {
    ...
  },

  // TODO: remove `insertAt`, `removeAt`, `pushObject` and `unshiftObject` when https://github.com/emberjs/ember.js/issues/17282 is fixed
  insertAt(idx: number, object: any): ISmartMutableArrayProxy<T> {
    ...
  },

  removeAt(start: any, len = 1) {
    ...
  },

  removeObject(object: any): ISmartMutableArrayProxy<T> {
    ...
  },

  pushObject(obj: any): any {
    ...
  },

  unshiftObject(obj: any): any {
    ...
  },
};

const result: ISmartMutableArrayProxy<T> = Mixin
  // @ts-ignore
  .create(MutableArray, Observable, properties)
  // @ts-ignore - without is private API
  .without(['length'])
  .apply(array);

As you can see, I'm just implementing a couple of methods in my own way and reusing most of the functionality in the Mixins. The implementations are kind of straight-forward so the performance shouldn't be (much) worse than a normal array, I guess?

Not sure what you meant by the last comment - is that a bug after all? Is your PR going to fix it or just remove some unused code? Any ideas how to workaround the problem after all?

[rwjblue]

@boris-petrov - Just to confirm, are you saying that this is a regression in 3.16? If yes, what was the last version that worked properly?

[rwjblue]

@boris-petrov

Just thinking about this some more, and I had a couple more questions:

• Does your custom EmberArray implementation extend from native array? (If not, the fix proposed in [BUGFIX release] Don't setup mandatory setters on array indexes #18741 won't fix your scenario)
• If you are using the Enumerable protocol, why are you also using direct a[0]? I would think that you need to use a.objectAt(0) to follow the protocol

[boris-petrov]

@rwjblue - I don't think this is a regression in 3.16 (although I'm not sure because I just found that bug).

About your other points:

1. I've posted pretty much the whole code in my second comment above - the relevant part being:

const result: ISmartMutableArrayProxy<T> = Mixin
  .create(MutableArray, Observable, properties)
  .without(['length'])
  .apply(array);

That is, I mixin MutableArray, Observable and an object (which has objectAt, replace and some other stuff) and apply that to a normal JavaScript array. This is my "smart mutable array proxy" as I've named the type. So I don't "extend" anything, I mixin stuff to the native JS array.

2. In my case I save in the array directly the values that I want returned (i.e., when doing set ot replace or pushObject or something like that I modify the passed value and set in the array directly the "correct" value) so my implementation of objectAt is pretty much return this[index] - hence I don't have to use objectAt and can directly use it as an array. That was on purpose to preserve the "native" look and feel of this array proxy.

As for whether that PR will fix my problem... no idea. If it fixes the example in my first post, probably it will fix my use case too. I'll wait for a new release and try it out and I'll let you know. :)