Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Deprecate array prototype extensions #848

Merged
Merged
Changes from 13 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
178 changes: 178 additions & 0 deletions text/0000-deprecate-array-prototype-extensions.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,178 @@
---
Stage: Initial
Start Date: 2022-8-21
Release Date: Unreleased
Release Versions:
ember-source: vX.Y.Z
ember-data: vX.Y.Z
Relevant Team(s): Ember.js
RFC PR:
---

# Deprecate array prototype extensions

## Summary

This RFC proposes to deprecate array prototype extensions.

## Motivation

Ember historically extended the prototypes of native Javascript arrays to implement `Ember.Enumerable`, `Ember.MutableEnumerable`, `Ember.MutableArray`, `Ember.Array`. This added convenient methods and properties, and also made Ember arrays automatically participate in the Ember Classic reactivity system.

Those convenient methods increase the likelihood of becoming potential roadblocks for future built-in language extensions, and make it confusing for users to onboard: is it specifically part of Ember, or Javascript? Also with Ember Octane, the new reactivity system, those classic observable-based methods are no longer needed.

We had deprecated [Functions](https://github.com/emberjs/rfcs/blob/master/text/0272-deprecation-native-function-prototype-extensions.md) and [Strings](https://github.com/emberjs/rfcs/blob/master/text/0236-deprecation-ember-string.md) prototype extensions. Array is the last step. And internally we had already been preferring generic array methods over prototype extensions ([epic](https://github.com/emberjs/ember.js/issues/15501)).

Continuing in that direction, we should consider recommending the usage of native array functions as opposed to convenient prototype extension methods, and the usage of `@tracked` properties or `TrackedArray` over classic reactivity methods.

## Transition Path

For convenient methods like `filterBy`, `compact`, `sortBy` etc., the replacement functionalities already exist either through native array methods or utility libraries like [lodash](https://lodash.com), [Ramda](https://ramdajs.com), etc.

For mutation methods (like `pushObject`, `removeObject`) or observable properties (like `firstObject`, `lastObject`) participating in the Ember classic reactivity system, the replacement functionalities also already exist in the form of immutable update style with tracked properties like `@tracked someArray = []`, or through utilizing `TrackedArray` from `tracked-built-ins`.

We don't need to build anything new specifically, however, the bulk of the transition will be
focused on deprecating the existing usages of array prototype extensions.

## How We Teach This

An entry to the [Deprecation Guides](https://deprecations.emberjs.com/v4.x) will be added outlining the different recommended transition strategies. ([Proposed deprecation guide](https://github.com/ember-learn/deprecation-app/pull/1192))

Rule `ember/no-array-prototype-extensions` is available for both [eslint](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-array-prototype-extensions.md) and [template lint](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-array-prototype-extensions.md) usages. Rule examples have recommendations for equivalences.

We can leverage the fixers of lint rule to auto fix some of the issues, e.g. the built-in [fixer](https://github.com/ember-template-lint/ember-template-lint/blob/master/docs/rule/no-array-prototype-extensions.md) of `firstObject` usages in template.

We also should create codemods or autofixers in lint rules for some of the convinient functions like `reject`, `compact`, `any` etc. More discussions on **Unresolved Questions** section.

Examples (taken from [Deprecation Guide](https://github.com/smilland/rfcs/pull/1)):
### Convenient methods
Examples of deprecated and current code:
```js
import Component from '@glimmer/component';
import { uniqBy, sortBy } from 'lodash';

export default class SampleComponent extends Component {
abc = ['x', 'y', 'z', undefined, 'x'];

// deprecated
def = this.abc.compact();
ghi = this.abc.uniq();
jkl = this.abc.toArray();
mno = this.abc.uniqBy('y');
pqr = this.abc.sortBy('z');
// ...

// current
// compact
def = this.abc.filter(el => el !== null && el !== undefined);
// uniq
ghi = [...new Set(this.abc)];
// toArray
jkl = [...this.abc];
// uniqBy
mno = uniqBy(this.abc, 'y');
// sortBy
pqr = sortBy(this.abc, 'z');
};
```

### Observable properties and methods in js
Examples of deprecated code:
```js
import Component from '@glimmer/component';
import { action } from '@ember/object';

export default class SampleComponent extends Component{
abc = [];

// observable property
get lastObj() {
return this.abc.lastObject;
}

@action
someAction(newItem) {
// observable method
this.abc.pushObject(newItem);
}
};
```

Examples of current code.
#### Option 1: use `tracked` property
```js
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { tracked } from '@glimmer/tracking';

export default class SampleComponent extends Component{
@tracked abc = [];

get lastObj() {
return this.abc.at(-1);
}

@action
someAction(newItem) {
this.abc = [...abc, newItem];
}
};
```

#### Option 2: use `TrackedArray`
```js
import Component from '@glimmer/component';
import { action } from '@ember/object';
import { TrackedArray } from 'tracked-built-ins';

export default class SampleComponent extends Component{
abc = new TrackedArray();

get lastObj() {
return this.abc.at(-1);
}

@action
someAction(newItem) {
abc.push(newItem);
}
};
```

### Observable properties in templates
Examples of deprecated code:

```hbs
<Foo @bar={{@list.firstObject.name}} />
```

Examples of current code:
```hbs
<Foo @bar={{get @list '0.name'}} />
```

After the deprecated code is removed from Ember (at 5.0), we need to remove the [options to disable the array prototype extension](https://guides.emberjs.com/v4.2.0/configuring-ember/disabling-prototype-extensions/) from Official Guides and we also need to update the [Tracked Properties](https://guides.emberjs.com/v4.2.0/upgrading/current-edition/tracked-properties/#toc_arrays) `Arrays` section with updated suggestions.

## Drawbacks
- For users relying on Ember array prototype extensions, they will have to refactor their code and use equivalences appropriately.
- A lot of existing Ember forums/blogs had been assuming the enabling of array prototype extensions which could cause confusions for users referencing them.
- Increase package sizes, for example, before `this.abc.filterBy('x');`, now `this.abc.filter(el => el !== 'x');`.
- Although `tracked-built-ins` is on the path to stabilization as an official API via [RFC #812](https://github.com/emberjs/rfcs/pull/812), it is not yet officially recommended and its API may change somewhat between now and stabilization.

## Alternatives
- Do the deprecation as suggested here for use within Ember itself, but extract it as a standalone library for users who want to use it. This will only work as long as the underlying Ember Classic reactivity system is supported.

As a variation on this, we could do this but explicitly only support it up through the first LTS release in the 5.x series.

- Continuing allowing array prototype extensions but turning the `EXTEND_PROTOTYPES` off by default.

- Do one of these, but target Ember v6 instead.

- Do nothing.

## Unresolved questions
- Current lint rule [ember/no-array-prototype-extensions](https://github.com/ember-cli/eslint-plugin-ember/blob/master/docs/rules/no-array-prototype-extensions.md) gives a lot of false positives giving the limitation of static analysis.
smilland marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these false positives will be greatly reduced soon because in ember-data 4.8 usages of these has already been deprecated. This means remaining usage is from A() which I would hope this RFC would consider simultaneously deprecating or various other ArrayProxy implementations which are far less common.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed! One of the things I plan to discuss at Friday's Framework Core meeting is exactly whether we should deprecate @ember/array entirely and/or at least extracting it and move it out to a separate package which is not installed by default.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but the reason I mention A is that A mutates array instances when prototype extensions are turned off. If we're going to rip off the bandaide it seems better to rip it fully off vs allowing people to just call A on everything to get back to the same state.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another sets of false positives are self-defined class functions, for exampleinvoke, clear are pretty common method names. ☹️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@runspired yeah, I spent literally my whole day today dealing specifically with the horror caused by the NativeArray shenanigans as relates to types. If it were my preference alone, we'd deprecate it and cut a special v5.0 tomorrow just to get rid of A(). 😂

@smilland yeah, unfortunately that kind of thing is basically impossible to avoid 100% without full TS coverage (which we obviously can't require people to have!).

- Difficulties for providing stable codemods or autofixers:
1. giving false positives on lint rules, same will apply to codemods or autofixers;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assuming that the lint rule and codemod would both rely solely on static analysis (i.e. the codemod would not have any runtime information available) (previous thread), then personally I'd rather focus on building out the ember/no-array-prototype-extensions lint rule autofixer, as that would be easily accessible to all Ember users who already have eslint-plugin-ember installed and already integrated into their IDE/tooling, as opposed to a codemod which requires someone to seek it out and manually run it.

2. when migrating certain methods, we need to access object. Shall we use Ember `get` or native way? Unless we fully remove ObjectProxy dependency, codemods or autofixers would still require manual work in certain cases.
3. observable functions or properties requires manual refactoring;