Skip to content

Deprecates Ember.ArrayProxy#12843

Closed
trek wants to merge 1 commit intoemberjs:masterfrom
trek:deprecate-array-proxy
Closed

Deprecates Ember.ArrayProxy#12843
trek wants to merge 1 commit intoemberjs:masterfrom
trek:deprecate-array-proxy

Conversation

@trek
Copy link
Member

@trek trek commented Jan 20, 2016

Includes a legacy flag for Ember Data to continue to use Ember.ArrayProxy internally.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, this isn't better on ENV maybe? Ala Ember.ENV._ENABLE_LEGACY_CONTROLLER_SUPPORT (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 believe this is correct. Basically, we are only deprecating when not used via Ember Data. This is on a per instance basis, not a global ENV basis.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, gotcha. 👍

@mixonic
Copy link
Member

mixonic commented Jan 20, 2016

Can you flag the tests with [DEPRECATED] at the end of the names? This is sweet.

@rwjblue
Copy link
Member

rwjblue commented Jan 20, 2016

Can we also get an entry in emberjs/website's deprecation guide?

@btecu
Copy link
Contributor

btecu commented Jan 20, 2016

Why is this being deprecated?
Is there an alternative?

@rwjblue
Copy link
Member

rwjblue commented Jan 20, 2016

@btecu - Great question! That is exactly what the deprecation guide entry should point out.

@cibernox
Copy link
Contributor

I'm also curious about the reason. I use it in a couple places in conjunction with PromiseProxyMixin. Is it for performance reasons?

@miguelcobain
Copy link
Contributor

Why is this being deprecated?

😱

@trek
Copy link
Member Author

trek commented Jan 21, 2016

@btecu @cibernox can you tell us your use cases? It will help us write alternatives in the deprecation guide.

@workmanw
Copy link

I can't speak for @btecu or @cibernox but my use case is I have a custom QueryModelArray that I've implemented in my application which is modeled after ember-data's RecordArray. My implementation allows for infinite paging and a couple of other domain specific helpers.

I don't have to use Ember.ArrayProxy per say, I'm just following the pattern laid out by ember-data. I saw it mentioned that "Includes a legacy flag for Ember Data". As long as I can use that in my app as well, I'm happy.


One other note, we DO use Ember.ObjectProxy for buffered proxy patterns. I hope that guy isn't slated for deprecation.

@btecu
Copy link
Contributor

btecu commented Jan 21, 2016

I have a dynamic table component and in some cases I want to display different data than the received. So if the value received from the api is yes, I might want to display a check mark instead. In some cases and this depends on the specific model I'll display a specific icon or value depending on the value and column. This is specifically UI and it doesn't belong in the api.

So I use ArrayProxy for the entire model and then inside (objectAtContent) I use ObjectProxy for each column of the table that I need customized.

@cibernox
Copy link
Contributor

The use case I'm aware now, is using ArrayProxy in conjunction with PromiseProxyMixin like this:

const PromiseArray = Ember.ArrayProxy.extend(Ember.PromiseProxyMixin);

export default Ember.Component.extend({
  results: computed('foo', function() {
    // ...
    return PromiseArray.create({ promise, content: previousResults });
  })
})

That basically gives me a collection-like object that I can iterate that contains some initial elements and also has a isPending property set to true. Once the promise resolves, the content of the array proxy is replaced by the resolved value of the promise (and isPending becomes false).

I'm not saying this shouldn't be deprecated, I already have rough idea in my head about how to stop using it, I just was curious about the reasons. Performance? I understand there must be some penalty, but I never measured it.

@raytiley
Copy link
Contributor

I'm doing basically the same pattern as @cibernox in several places in my app.

@miguelcobain
Copy link
Contributor

Same here.

@alexdiliberto
Copy link
Contributor

Just to note, I have converted some ArrayControllers over using this pattern, however, I believe Ember.computed.sort() is the more viable alternative.

  foo: Ember.computed('model', function() {
    return Ember.ArrayProxy.extend(Ember.SortableMixin).create({
      sortProperties: ['bar'],
      sortAscending: false,
      content: this.get('model')
    });
  }

@barneywilliams
Copy link

We use it for a few cases where we just want to be notified of an item being added/removed from a collection. The issue is that when a computed prop is dependent on someCollection.[] that you only get a the CP re-evaled. Figuring out which element was added/removed requires a cache implementation to diff against.

This is similar to the Collection add/remove events in Backbone, which is one thing I missed when moving to Ember.

We have also use this type of functionality to insert and item into a sorted list at the right spot, rather than doing a full sort on a full set of data. We have experienced very significant perf gains on using this for large sets of data.

@stefanpenner
Copy link
Member

@barneywilliams do you mean an array? Or some arbitrary collection. If it's just an array, normal arrays already track such changes

@toranb
Copy link
Contributor

toranb commented Jan 21, 2016

I'm also interested in why this is going away - one of my data store libraries is using this heavily like ember-data was for basic find/fetch/filtering of models in the store

export default ArrayProxy.extend({
    store: null,
    type: null,
    content: computed(function () {
        return Ember.A(this.get("source"));
    })
});

update

Use case 1 (model hook from the route)

  1. return an ArrayProxy directly from my route (no waiting for a promise to resolve so I can paint quickly)
  2. when the promise dispatched in step 1 is resolved my ArrayProxy will update it's content property (at the end of that run-loop)
  3. any template bound to that original (empty) ArrayProxy is now notified of a change and updated

Use case 2 (relationships)

Below is how I currently model a one-to-many relationship using ArrayProxy at the center of it all

var User = Model.extend({
    role: Ember.computed.alias('belongs_to.firstObject'),
    belongs_to: Ember.computed(function() {
        const user_id = this.get('id');
        const store = this.get('store');
        const filter = function(role) {
            const users = role.get('users');
            return Ember.$.inArray(user_id, users) > -1;
        };
        return store.find('role', filter, ['users']);
    })
})

And what the unit test for that relationship looks like (to see it in action)

test('role returns associated model or undefined', function(assert) {
    let user = store.push('user', {id: 1});
    store.push('role', {id: 2, name: 'Admin', users: [1]});
    let role = user.get('role');
    assert.equal(role.get('id'), 2);
    assert.equal(role.get('name'), 'Admin');
    store.push('role', {id: 2, users: []}); //update users on role to trigger a change
    role = user.get('role');
    assert.equal(role, undefined);
});

When the ArrayProxy content is updated my "filter" above will recompute the ArrayProxy for "belongs_to" allowing me to change roles in the system for a given user

@barneywilliams
Copy link

@stefanpenner I was referring to collections in general.. but arrays are usually what I have dealt with. i know that you can get a notification on when an array changes in size with 'someArray.[]', but that doesn't let you know what item(s) were added/removed... unless there is a trick I don't know about?

@stefanpenner
Copy link
Member

@barneywilliams although they are very low level, and must be used with care array observers allow for this.

@stefanpenner
Copy link
Member

@toranb makes a good point, until we can subclass arrays decorating them is likely one of the better options. This doesn't mean these proxies can't be extracted, but we should take this into account.

@eviltrout
Copy link
Contributor

We use Array Proxies in Discourse and we don't use Ember Data. We have our own lightweight data API on top of Ember and we use it for ResultSet objects like @toranb does.

We also use it in a half dozen other places based on a quick grep. I'm also curious what the rationale for this deprecation is?

@stefanpenner
Copy link
Member

I'm also curious what the rationale for this deprecation is?

I believe the concern is that it is a leaky abstraction, but maybe those concerns need to be fully enumerated. Maybe we can address those issues.

I wonder if a counter balance should be applied to the kill all proxies with fire. I believe stateful proxies, or proxies that leak their abstraction are the pain point. We should likely explore this in more detail.

@xkb
Copy link

xkb commented Jan 21, 2016

I've used ArrayProxy with PromiseProxyMixin to implement @toranb's "Use case 1" which I beleive what will async routable components provide in the future?

Only way I've found to synchronously load a template and then update the array proxy when the actual promise resolves. Pretty much a DS.PromiseArray.

@stefanpenner
Copy link
Member

which I beleive what will async routable components provide in the future?

nah, thats orthogonal

@stefanpenner
Copy link
Member

Only way I've found to synchronously load a template and then update the array proxy when the actual promise resolves. Pretty much a DS.PromiseArray.

I would like to (before removing those constructs) make the underlying system correctly understand promises. So ultimately making them pointless before removing them.

@pixelhandler
Copy link
Contributor

Please please, do not deprecate ArrayProxy or ObjectProxy these are super useful for promise proxy objects. Seriously there is a lack of diversity among data solutions in the Ember ecosystem. These three objects are critical to developing data persistence solutions.

👎

So if Ember Data needs these objects, why deprecate for other people not using Ember Data, is that the only data persistence solution that is "blessed" to use these primitives? Will The PromiseProxyMixin, ArrayProxy and Object proxy become an Addon combo for Ember Data and any data persistence solution to utilize?

@pixelhandler
Copy link
Contributor

@trek please add a checkbox and issue for a "way forward" perhaps "[ ] Create Addon for PromiseProxyMixin, ArrayProxy and Object Proxy"

@rwjblue
Copy link
Member

rwjblue commented Jan 21, 2016

@pixelhandler - PromiseProxyMixin and ObjectProxy are completely unrelated to this PR.

@pixelhandler
Copy link
Contributor

@rwjblue ok so how about "[ ] Create Addon for ArrayProxy"

@pixelhandler
Copy link
Contributor

@toranb Per your suggestion about sharing use cases…

I use the ArrayProxy (and ObjectProxy) with the PromiseProxyMixin as a utility for related resources in a data persistence library like so: https://github.com/pixelhandler/ember-jsonapi-resources/blob/master/addon/utils/related-proxy.js#L50-L73 and https://github.com/pixelhandler/ember-jsonapi-resources/blob/master/addon/utils/related-proxy.js#L90-L92

I would be happy to use these same objects if they lived in an Ember CLI addon and no longer were included as primitives in Ember.js proper. I think they are valuable to anyone working with data persistence.

I am curious what the proposed "way forward" is?

@stefanpenner
Copy link
Member

One thing worth noting, is all basic proxies like promise + object make removing the use of .get basically not possible, or at least not a 👣 🔫

@stefanpenner
Copy link
Member

I am curious what the proposed "way forward" is?

Well, for example: One could decorate an array with additional functionality. making it one with the proxy, rather then indirectly a proxy. This is more or less one path nicely aligned with the platform, especially once we are able to subclass native arrays cross platform. Unfortunately until real subclassing is possible that approach isn't really that best. :(

@opsb
Copy link

opsb commented Jan 22, 2016

We're using it to implement live queries and hasMany relationships in ember-orbit.

It's not something that I often reach for but when needed it is very useful. I'm not sure what the reasoning is behind it's removal but I do see a couple of issues that could perhaps be addressed.

  1. content should really be private
  2. It assumes the ArrayProxy will be mutable, in the case of a result set it should be readonly (we worked around this with ReadOnlyArrayProxy).

@ghost
Copy link

ghost commented Jan 22, 2016

I use the ArrayProxy in combination with unknownProperty & setUnknownProperty to make an array of DS.Model instances act as a single object while maintaining being an array of things.

Example: setting the 'admin' property on the array proxy will set the 'admin' property on all items in the array proxy.

@fivetanley
Copy link
Member

ArrayProxy is something we'd (or at least I'd) like to move away from in Ember Data. Or at least our current usage of it. I don't have an alternate implementation, but an ArrayProxy that had a hidden content (even if just _content), didn't support willChangeContent/didChangeContent or arrangedContent would make me pretty happy as the implementations there give us many bug reports.

I don't know if we'd be able to drop it because of backwards compat, but removing the most troubling aspects of ArrayProxy would help a lot.

@trek
Copy link
Member Author

trek commented Jan 29, 2016

All: This was great! You might have seen in the notes on the last face to face that the core team is trying to trickle out deprecations on a more, ahem, tolerable time table during this major revision.

Part of that process means doing PRs for deprecations early so we can get community comments. I think the core team were occasionally surprised at some of the creative uses of public 1.x API and want to do a better job ensuring alternate APIs exist for all use cases this time around.

ArrayProxy likely won't be in 3.0, but we won't merge this PR until we've addressed the concerns here.

@btecu, et al

Why is this being deprecated?

ArrayProxy (and proxies in general) make more sense in the 1.x/2.x programming model. Glimmer and other framework changes are going make them mostly unnecessary in 3.x

The three main uses for ArrayProxy that have come up are

  1. Extra features and observable aware methods on Array-like objects

    As @stefanpenner mentions, we already extend regular Array objects to track mutations with methods like pushObject. The alternative is to use Array.

  2. Maintaining sort order/filtering/etc

    For behaviors like maintaining sort order you can use Ember.computed.sort(), Array#sortBy or replace the existing array with a new array that contains the items you want, and sort yourself:

    actions: {
      addAnItem(item) {
        let array = this.get('items').slice(); // <- creating a new array, VERY IMPORTANT.
        array.push(item);
        array.sort(someCompareFunction);
        this.set('items', array);
      }
    }
    
  3. Presenting a stable reference for model() resolution as part of a data layer.

    As @toranb et al mention having a stable ref allows for immediate resolution of model hooks. Although not all specifically enumerated, the underlying problems this approach address:

    • Manual control of loading states. I'm guessing that this desire comes from wanting to paint some the view for a route, just not portions of the view that rely on the contents of the collection being loaded.
    • Allows model hooks to be non-blocking, so child hooks that don't need access to an ancestor's model can trigger immediately.

    We still need to decide on patterns for handling the above two. The solutions will depend on the specifics on additions coming future 2.x releases (like engines, routable components).

    The solution might still be something Proxy-like, but that solution will drop methods unrelated to being a stable ref. Namely: content would be private, no arrangedContent, no willChangeContent/didChangeContent, filtering and sorting would be resposibilities of your data layer or view layer.


Instead of deprecating ArrayProxy, we could deprecate all the features that are better handled elsewhere, but that guts ArrayProxy so much that it effectively becomes a new class.

To aid migration, would having multiple deprecation warnings per ArrayProxy be helpful? That is, would it be useful to know where you were using each of the use cases above?

@opsb
Copy link

opsb commented Jan 29, 2016

The solution might still be something Proxy-like, but that solution will drop methods unrelated to being a stable ref. Namely: content would be private, no arrangedContent, no willChangeContent/didChangeContent, filtering and sorting would be resposibilities of your data layer or view layer.

This solution will solve it, but I thought I'd reiterate the scenario that I'm considering:

In orbit.js we stream updates to live queries(queries whose results are updated in realtime) through an Observable (we're using RxJS under the hood). Ember-orbit uses an ArrayProxy to provide a reference to the latest array of results received from this Observable.

@runspired
Copy link
Contributor

I use a few custom array proxies as well, but I do often reach for this. It's an incredibly useful tool. I think it would make sense at least to provide this as an addon.

Like @opsb is using it, I also do not rely on the proxy to arrange / sort / filter the content. However, unlike him I not only use it as a stable reference, but also for running granular operations when content changes. It's much more stable than didReceiveAttrs for this, because didReceiveAttrs only triggers for array content changes when the entire reference changes or when the underlying array is an Ember.Array, not a native array.

@raytiley
Copy link
Contributor

raytiley commented Feb 2, 2016

const PromiseArray = Ember.ArrayProxy.extend(Ember.PromiseProxyMixin);

export default Ember.Component.extend({
  results: computed('foo', function() {
    // ...
    return PromiseArray.create({ promise, content: previousResults });
  })
})

@trek forgive me if I missed something but I don't specifically see a way forward in your reply for this use case. The major place we are still using ArrayProxy in our app is to return array like objects from a computed property that can be used in {{each}}.

This seems to always be a thing that comes up but there isn't a best practice that I am aware of. Maybe it would make more sense to just return a promise from the CP that will resolve into an array, and make {{each}} yield to the inverse if it is passed an in flight promise object?

@homu
Copy link
Contributor

homu commented Feb 6, 2016

☔ The latest upstream changes (presumably #12914) made this pull request unmergeable. Please resolve the merge conflicts.

@trek
Copy link
Member Author

trek commented Feb 8, 2016

@raytiley yeah, we generally need better handling for async flow control at the template layer. I'm not sure who suggested it (@stefanpenner maybe?), but an {{await someProperty}} helper might fill the same use you need here?

@stefanpenner
Copy link
Member

Ya, I believe many of the cases described here can be replaced with tools that are better suited for the task. Although those may not all be ready yet, hopefully soon they will be.

@jrjohnson
Copy link

We've been using ember-data's DS.PromiseArray to do the work @raytiley is talking about in #12843 (comment)

So I wasn't too worried about this given the caveat that data would be excepted. However it appears that PromiseArray is actually private in data. Deprecating this doesn't bother me, but before that happens there should be a shared solution to the common problem of needing to put together a set of one or more promises into an array to get passed into a template. Right now it is dirt simple to simple return a DS.PromiseArray from a CP which {{each}} will use and which can also be tested for isFulfilled to provide for a nice loading indication or pre-loaded state.

Maybe it is making DS.PromiseArray public warp-drive-data/warp-drive#4163 ?

@pixelhandler
Copy link
Contributor

pixelhandler commented Nov 18, 2016

@trek Is this PR inactive? What is the path forward? To deprecate or not to deprecate?

Perhaps this should be closed in favor of an RFC issue.

@Serabe
Copy link
Member

Serabe commented Jul 13, 2017

Closing this PR as inactive. Thank you!

@Serabe Serabe closed this Jul 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.