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

Already on GitHub? Sign in to your account

Possibly add `isLoaded` to `DS.PromiseArray` #1274

Closed
sandstrom opened this Issue Sep 11, 2013 · 15 comments

Comments

Projects
None yet
5 participants
Contributor

sandstrom commented Sep 11, 2013

On DS.RecordArray there is a isLoaded flag, however the DS.PromiseArray has nothing of that sort.

There is a flag isSettled and isFullfilled, but apart from being promise-specific, when they fire the array doesn't have any length, e.g. still empty from the perspective of the UI/application.

Is this something that should be added to DS.PromiseArray?


Common use-cases are loading indicators, or reading metadata from the type map, e.g.

Em.computed(function() {
  return this.get('store').typeMapFor(model).metadata.pagination.count;
}, 'collection.isLoaded');
Owner

wycats commented Sep 12, 2013

@stefanpenner was persuaded to remove isLoading. Perhaps he can explain his rationale?

(btw, there is nothing wrong with async flags being promise-related, as Ember uses promises for all async handling).

Owner

stefanpenner commented Sep 12, 2013

isPending should be available, and means something similar. The PromiseProxy, tries to stick with the language of promises.

https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/lib/mixins/promise_proxy.js#L91

Owner

wycats commented Sep 12, 2013

@stefanpenner I think the concern is that isLoaded is already an Ember idiom for general async stuff.

Contributor

sandstrom commented Sep 12, 2013

@stefanpenner In the use case I had in mind (loading indicators) isPending doesn't seem to work, because it fires/changes earlier than isLoading did.

My current work-around is to use {{#unless content.firstObject.id}} loading... {{else}} ... {{/unless}}, which has about the same effect as isLoaded (unless 0 records are returned, then it's an infinite loading indicator).

Also, perhaps isPending can co-exist with isLoading, using isLoading as a generic property, to be used both for Promises, but also for say, the array returned by store.all('foo') (which is returned immediately), a ManyArray (which I guess may not be a promise if all associations are already in the store?).

Does anything of this resonate? 😃

Owner

stefanpenner commented Sep 12, 2013

So "isLoading" has nothing to do with the promise settling, rather it has to do with the detecting that at least one object exists in the array?

Contributor

sandstrom commented Sep 12, 2013

Well, I'm thinking isLoading can be used to toggle loading indicators (and isPending cannot be used for that as it stands now). So, I wanted to highlight the use case and hear your thoughts.

I'm thinking that isLoading can coexist with isPending, and that isLoading could also be used in synchronous situations, e.g. store.all('foo'). This is how it used to work until the recent beta.

It's not about detecting at least one object in the array, an array may be loaded but empty.

hajee commented Sep 12, 2013

The DS.RecordArray has the following implementation for isLoaded

DS.AdapterPopulatedRecordArray = DS.RecordArray.extend({
  ...
  load: function(data) {
    var store = get(this, 'store'),
        type = get(this, 'type'),
        records = store.pushMany(type, data);

    this.setProperties({
      content: Ember.A(records),
      isLoaded: true
    });

After all records have been fetched and loaded into the store, isLoaded is set to true. I guess there is some time between !isPending and isLoaded

Owner

stefanpenner commented Sep 12, 2013

Is the expectation that isLoading to flip on/off whenever the RecordArray is updating data?

Remember that a record array may initially load to an empty array, but later be populated. How it is populated, or that it may become populated, or that is is pending an interaction that may populate it, is often totally unknown.

It seems like you might want a mixture of isEmpty + isPending. To detect the initial realization of the RecordArray. Subsequent requests or actions that may populate the record Array will likely need to be the source of truth on this one.

Anyways, an example for us to base this discussion on:

// my interpretation of the scenario we are trying to solve (it may be wrong, so respond as needed)
{{#if user.comments.isSettled}}
  {{#each user.comments}}
      <h2> {{title}} </h2>
     <p>{{body}}<p>
  {{else}}
     no comments....
  {{/each}}
{{else}}
  loading spinner ....
{{/if}}
Contributor

sandstrom commented Sep 12, 2013

@stefanpenner Yes, I think we're thinking about this the same way, and I agree with your example.

(Don't understand the mixture of isEmpty and isPending part, in my view those typically come one after the other. First we're awaiting data from the server (or async local storage, or something else), when that data have arrived it may turn out to be an empty collection. But this feels like an aside; again I think we're on the same page overall -- also, my knowledge isn't nearly as good as yours on this topic)

hajee commented Sep 12, 2013

I use it:

{#if content.isLoaded}}
  {{#each controller itemController="project"}}
  {{/each}}
{{else}}
  {{dark-loading-indicator}}
{{/if}}

This used to show a loading indicator when the data is being fetched. Any new additions or updated to the RecordArray once it is fetched, don't do anything with isLoaded. Also a save on a loaded RecordArray doesn't change isLoaded .

hajee commented Sep 12, 2013

Looks like delegating to the content

DS.PromiseArray.reopen
    isLoaded:(->
        @get('content.isLoaded')
    ).property('content.isLoaded')

shows same behavior as before

Owner

wycats commented Sep 22, 2013

@stefanpenner I personally think that having a single "loading" flag that is agnostic to promises usage (even if it is implemented as !isSettled) would be helpful.

Owner

wycats commented Sep 23, 2013

Having re-read this thread, I really do not understand why checking for isPending doesn't satisfy the requirements. Can you provide a JSBin that illustrates the timing issue?

@wycats wycats closed this Sep 23, 2013

Contributor

sandstrom commented Sep 23, 2013

@wycats You're right, the timing issue is probably something else (don't have a JSBin unfortunately).

Regardless, it would be nice to have an agnostic isLoading flag — and you seem to agree :)

I never heard of isSettled, it seems to solve my problem. But the thing is, I've heard a lot of isLoading, so I tried to use that without success. It would be logical that isLoading also works for DS.PromiseArray

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment