Deprecate Ember.A #864
Deprecate Ember.A #864
Conversation
jrjohnson
force-pushed
the
deprecate-ember-a
branch
3 times, most recently
from
8733b3a to
8215cfa
Compare
jrjohnson
force-pushed
the
deprecate-ember-a
branch
from
8215cfa to
53b64e0
Compare
text/0864-deprecate-ember-a.md
Outdated
|
|
||
| ## 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. |
There was a problem hiding this comment.
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!
text/0864-deprecate-ember-a.md
Outdated
|
|
||
| `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. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
I think we may also be able to fix this, but it's definitely clear that Ember.A does weird stuff!
| } | ||
| ``` | ||
|
|
||
| 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. |
|
|
||
| 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. | ||
|
|
||
| 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. |
There was a problem hiding this comment.
This is a very good reason to deprecate Ember.A, ASAP.
There was a problem hiding this comment.
Credit to @runspired, I stole this from #848 (comment)
text/0864-deprecate-ember-a.md
Outdated
|
|
||
| ## 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. |
There was a problem hiding this comment.
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.
|
|
||
| #### 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
|
||
| ### 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()`. |
There was a problem hiding this comment.
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.
Co-authored-by: Peter Wagenet <peter.wagenet@gmail.com>
Co-authored-by: MrChocolatine <47531779+MrChocolatine@users.noreply.github.com>
|
Thanks @wagenet and @MrChocolatine, I've incorporated your feedback. |
Co-authored-by: MrChocolatine <47531779+MrChocolatine@users.noreply.github.com>
Co-authored-by: Bryan Mishkin <698306+bmish@users.noreply.github.com>
wagenet
added
S-Exploring
|
@wagenet / @chriskrycho anything I should be doing to push this along or advocate for the change? |
|
@jrjohnson Thanks for checking in on this. We talked about this in core today and I think this is going to have to be part of a more comprehensive plan for deprecating classic and @ember/component. I still think it's likely we'll be able to use this, but we just have to fit it in to the broader plan. |
|
@wagenet I don't quite understand why this deprecation would be held up by that process. I'm not worried about my work here, take it or leave it, but I am worried about the proliferation of |
|
My understanding is that there are still places where EmberArray still ends up being somewhat necessary, especially in the classic model. To have a deprecation without a full replacement is going to be pretty painful. I admit that I don't love our current situation, but my inclination is that it's actually better to risk more use of Ember.A in the short term if it means we can have a good plan to get rid of it properly in the long term. |
|
That makes sense for |
|
This was discussed this week in the feature design (aka spec) meeting where the suggestion was to keep Ember.A undeprecated during the transition away from the classic object model while deprecating all the actual non-standard array behaviors themselves. This could help ease the transition so that only code that's actively relying on the old system sees deprecations, and code that was treating Ember.A as a normal array won't need to be changed. |
rendered
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.