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

Deprecate Ember.A #864

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Changes from 1 commit
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
94 changes: 94 additions & 0 deletions text/0864-deprecate-ember-a.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
---
stage: accepted
start-date: 2022-11-22T00:00:00.000Z
release-date:
release-versions:
teams:
- data
- framework
- learning
- typescript
prs:
accepted: https://github.com/emberjs/rfcs/pull/864
project-link:
---

# Deprecate Ember.A()

## Summary

Remove `Ember.A()` as the functionality it provides is no longer needed in the post octane reactivity system and with the removal of array prototype extensions in #848.
Copy link
Member

Choose a reason for hiding this comment

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

I think we also may should deprecate Ember.NativeArray as part of this. I'd also like to deprecate Ember.Array altogether, but I'm not sure if that should be a separate thing or not. At the very least it would require the work we're doing here.


## Motivation

`Ember.A()` provides a way for addon authors or application owners who disable array prototype extensions to add reactivity to arrays (through `pushObject()`, `removeObject()`) and to access prototype extension convenience methods such as `filterBy()` on an any array-like object. Both of these paradigms have been deprecated in Ember and `Ember.A()` should follow suit as it can break things in unexpected ways and interferes with progress in [Ember](https://github.com/emberjs/ember.js/blob/4339725976299b24c69fb9dfbf13d18bf9917130/packages/@ember/-internals/utils/lib/ember-array.ts) and [Ember Data](https://github.com/emberjs/data/blob/47a71ca1538ba9e2d7dfa01bf048a2db897bdf5f/packages/store/addon/-private/record-arrays/identifier-array.ts#L381-L401) where special care must be taken to prevent or compensate for usage.
Copy link
Member

Choose a reason for hiding this comment

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

I think it may just be sufficient to say that maintaining behavior that diverges from a true native Array unnecessarily adds to maintenance and potential confusion. It's actually possible to make Ember.Array work and not cause problems for Ember Data (I think), but it would just be nicer to have it gone!

Copy link
Author

Choose a reason for hiding this comment

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


Along with it's usefulness `Ember.A()` has a significant API gotcha in that it doesn't return a new instance of the array-like object it is passed, it modifies that object and returns is such that `A(array) === array` this can create the dreaded spooky-action-at-a-distance effect where suddenly an object which was `Array` can become an `Ember.NativeArray`. An example of this is using `{{includes "Zoey" this.mascots}}` from [ember-composable-helpers](https://github.com/DockYard/ember-composable-helpers#includes) will modify `this.mascots` and add `lastObject` to the API. Re-ordering the template code at a later date will result in a mysterious failure that can be extremely frustrating to understand.
Copy link
Member

Choose a reason for hiding this comment

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

I think we may also be able to fix this, but it's definitely clear that Ember.A does weird stuff!


```js
export default class WeirdArray extends Component {
mascots = ['Tomster', 'Zoey'];

get lastMascot() {
return this.mascots.lastObject;
}

<template>
{{#if (includes "Zoey" this.mascots)}}
Zoey Rules!
{{/if}}
Last Mascot: {{this.lastMascot}}
</template>
}
```

Deprecating `Ember.A()` will provide a signal to the addon community to move away from both array prototype extensions and array reactivity which requires using the Emberism `pushObject`/`removeObject` to track updates.
Copy link
Member

Choose a reason for hiding this comment

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

💯


Now that array prototype extensions have been deprecated it is quite possible that usage of `Ember.A()` will increase significantly as a defensive coding strategy to maintain access to the family of methods Ember adds to all arrays. This should be immediately discouraged as `Ember.A()` is not a long term viable solution and other better alternatives exist.
Copy link
Member

Choose a reason for hiding this comment

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

This is a very good reason to deprecate Ember.A, ASAP.

Copy link
Author

Choose a reason for hiding this comment

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

Credit to @runspired, I stole this from #848 (comment)


## Transition Path

### Replacement APIs

As discussed in #848 better alternatives exist for the APIs provided by `Ember.A()`:

> 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`.

### Clearly Define Idiomatic Path

Deprecating `Ember.A()` will provide a clear signal to move away from this construct and rely on these more robust modern alternatives.

### Transitional Feature Flags

Transition away from `Ember.A()` should be aided by a pair of feature flags:

#### Wrap Passed Value

Instead of returning the passed value `Ember.A()` would return a native proxy to wrap the original object without modifying it. This would remove the most confusing part of the API and allow apps to opt in early and clear any unexpected issues before `Ember.A()` is fully removed.
Copy link
Member

Choose a reason for hiding this comment

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

I've started doing some of this work in emberjs/ember.js#20245. This change shouldn't actually require a flag since it's how Ember.A claims to work. However, we could add a short-lived flag to make it easier to validate the behavior in some real apps. Once we successfully validated we'd just remove the flag and have it always on. I don't think we'd have to publicize it in this case.

Once we successfully validate this, I'd like to add individual deprecations to the custom methods on the wrapper. That would help us to quickly identify cases where we're actually relying on the Ember.A behavior.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not sure if I need to express this in the RFC or if this is just for information / reference. I agree with most of it, though I don't share your faith that no one is relying on this behavior. I think it is probably fair to say that the number of apps intentionally relying in this is low, however I think the side effect may have turned into a feature for many.


#### Noop Ember.A

Currently if array prototype extensions is enabled `Ember.A()` is a `noop` and returns the passed value unmodified. It should be possible for application developers to turn off array prototype extensions and maintain this behavior of `Ember.A()`. This will make bugs easier to find and allow applications to more quickly prevent prototype pollution of the `Array` API before `Ember.A()` is fully removed.


## How We Teach This

`Ember.A()` is not covered in the guides, but it is present in [many](https://emberobserver.com/code-search?codeQuery=import%20%7B%20A%20%7D%20from%20%27%40ember%2Farray%27%3B) [many](https://emberobserver.com/code-search?codeQuery=Ember.A) addons.

Usage of convenience APIs can be easily replaced using the [no-array-prototype-extensions](https://github.com/ember-cli/eslint-plugin-ember/blob/4820f0fb286e40872a77d687618be56999f23704/docs/rules/no-array-prototype-extensions.md) codemod.

For reactivity addon authors will need to rely forcing tracking as `this.mascots = [...this.mascots, 'Rock & Roll']` or through utilizing `TrackedArray` from [tracked-built-ins](https://github.com/tracked-tools/tracked-built-ins).

## Drawbacks

`Ember.A()` has been a huge part of building Ember Addons for a long time and it is present in many popular addons. Removing this feature will cause a significant amount of churn in otherwise stable addons and require a significant community effort to replace.

## Alternatives

### Change Confusing API

Instead of modifying the passed value `Ember.A()` could return a new object which contains a reference to the original value. This would avoid the `A(array) === array` issue and make it easier to detect if an object had been passed through `A()` and to work with the original value if necessary. This would still be a breaking change as many apps (often unknowingly) rely on the behavior of an object having been passed through `A()`.
Copy link
Member

Choose a reason for hiding this comment

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

I think we're already planning on doing this. It wouldn't be considered breaking since the docs always said that Ember.A returns a new object. I think (I hope) it's rare for people to actually rely on the mutating behavior.


## Unresolved questions