Add deprecation guide for array prototype extensions for V5

[smilland]

RFC: Deprecate array prototype extensions

Rendered content

It's ready for review, though only can be merge after the RFC is approved.

[netlify]

✅ Deploy Preview for ember-deprecations ready!

Name	Link
🔨 Latest commit	25d831d
🔍 Latest deploy log	https://app.netlify.com/sites/ember-deprecations/deploys/66917b07f057110008f621f4
😎 Deploy Preview	https://deploy-preview-1192--ember-deprecations.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[tniezurawski]

In general, I think that it would have to be stated here that this implementation is not as robust as Ember Array's .sortBy. It has field names hardcoded and can't take an arbitrary number of field names.

[ef4]

We could also give people a replacement function they can paste into their own code. This is adapted from ember's own implementation:

import { compare } from '@ember/utils';
function sortBy<T>(array: T[], ...sortKeys: string[]): T[] {
  return this.toSorted((a: T, b: T) => {
    for (let i = 0; i < sortKeys.length; i++) {
      let key = sortKeys[i];
      let propA = a[key];
      let propB = b[key];
      let compareValue = compare(propA, propB);
      if (compareValue) {
        return compareValue;
      }
    }
    return 0;
  });
}

[tniezurawski]

I never used this method but the docs are saying:

Simply converts the object into a genuine array. The order is not guaranteed. Corresponds to the method implemented by Prototype.

So I guess an example should show how to convert an object into an array. I'd have to check how the original function works but would this be expected?

const person = {
    firstName: 'John',
    lastName: 'Doe'
};

Object.entries(person); // [ [ 'firstName', 'John' ], [ 'lastName', 'Doe' ] ]

🤔

[smilland]

thank you so much for reviewing @tniezurawski !

[elgordino]

I'm a little confused by the difference between this example and the pushObject example above which uses TrackedArray. Is it a requirement to assign to this.abc when using @tracked but not when using TrackedArray? Or is the different method shown to indicate that both approaches are valid?

[simonihmig]

yeah, @tracked will only trigger the reactivity when a new value gets assigned. When the existing native array is internally mutated, like calling .push(), then this will not be reactive. In opposite to TrackedArray, which does this kind of deep tracking. So I think the example is right.

[elgordino]

Would this.abc.push(...values) work here? It would be simpler and match the first after example for pushObject above?

[simonihmig]

yeah, i think so!

[simonihmig]

Lots of details in that migration guide, great work @smilland! I know this has been quite a whole since you worked on this, so would you still be available to look into and work on the provided suggestions?

The RFC was accepted long ago, so I think we can and should get this merged asap, as I would like to also get the actual deprecation implemented before we miss the time window for the v6 release!

[simonihmig]

Suggested change

	[new Person('Tom'), new Person('Joe')].map(person => person['greet']?.('Hi')); // ['Hi Tom', 'Hi Joe']
	[new Person('Tom'), new Person('Joe')].forEach(person => person.greet('Hi')); // ['Hi Tom', 'Hi Joe']

I think forEach is more appropriate than map here, since we don't use the value returned by map. Also the method call can be simplified as suggested I believe? invoke might do that optional chaining internally, but if people were to rewrite that code to native, I think this is what they would/should write.

[ef4]

invoke returns a value like map, so even though this example doesn't motivate using the return value I think it's correct to tell people to use map.

[simonihmig]

Is this example even needed? I think you would use toArray an an array-like object like A(), to turn it into a native array. But here we are only dealing with native arrays, so what's the point of toArray() usage then?

[simonihmig]

yeah, @tracked will only trigger the reactivity when a new value gets assigned. When the existing native array is internally mutated, like calling .push(), then this will not be reactive. In opposite to TrackedArray, which does this kind of deep tracking. So I think the example is right.

[simonihmig]

yeah, i think so!

[ef4]

Two other general notes that would be nice to include in this guide:

1. When the array prototype extensions are off, passing a native array to import { A } from '@ember/array' mutates the native array to re-add the prototype extensions. This can cause surprising behavior. (If somebody is excited to also deprecate A it's worth doing too.)
2. When the array prototype extensions are off, if you take an array-like value from ember-data and pass it to A, it breaks.

Other than these suggestions, this looks good to go. 👍

[ef4]

toSorted is new and it's a non-mutating version of sort:

Suggested change

	[...someArray].sort((a, b) => {
	someArray.toSorted((a, b) => {

[ef4]

We could also give people a replacement function they can paste into their own code. This is adapted from ember's own implementation:

import { compare } from '@ember/utils';
function sortBy<T>(array: T[], ...sortKeys: string[]): T[] {
  return this.toSorted((a: T, b: T) => {
    for (let i = 0; i < sortKeys.length; i++) {
      let key = sortKeys[i];
      let propA = a[key];
      let propB = b[key];
      let compareValue = compare(propA, propB);
      if (compareValue) {
        return compareValue;
      }
    }
    return 0;
  });
}

[ef4]

This deprecation is activated in ember beta 5.10, so it would be great to get this landed. I think only a small amount of minor fixes are needed here to get this green and mergeable.

[ef4]

I don't have permissions to do the Percy approval but it passes a visual inspection. Merging, because this deprecation is already released and we need a non-broken link.