Urigo / angular-meteor

Angular and Meteor - The perfect stack
https://www.angular-meteor.com/
MIT License
2.36k stars 622 forks source link

$meteor.subscribe was deprecated but there is no replacement with promise #1291

Closed trajano closed 8 years ago

trajano commented 8 years ago

The $meteor.subscribe function had been deprecated but it does not appear to have an equivalent that supports promise as the return value, the only one that is not deprecrated returns the subscribe handle but ui-router resolve would require promises

Deadly0 commented 8 years ago

@trajano you can create promise by yourself and resolve it in onReady subscription event.

trajano commented 8 years ago

Is there an example of that? And if so can it be added to the documentation for the deprecation?

Deadly0 commented 8 years ago

I use it like this:

    deferred = null
    $scope.subscribe 'collection', () ->
        if deferred
            deferred.resolve()

        deferred = $q.defer()
        $scope.dataLoading = deferred.promise

        perPage = parseInt $scope.getReactively('perPage')
        page = parseInt $scope.getReactively('page')
        $scope.options.limit = perPage
        $scope.options.skip = (page - 1) * perPage

        [$scope.getReactively('filter', true), $scope.options]
    , {
        onReady: () -> deferred.resolve()
        onStop: (err) -> deferred.resolve() if err
    }
trajano commented 8 years ago

Problem is $scope is not available on the resolve of ui-router. It won't make sense to use $rootScope because that means that subscription will always be active even if the state is not loaded.

trajano commented 8 years ago

Anyway I found a way of doing it

  resolve: {
    ... findSelector, findSortOption and findOptions resolvers ...
    initialDisplayedUsers: function($q, findSelector, findOptions, findSortOption) {
      return $q((resolve) => {
        var handle = Meteor.subscribe('user-list-admin', findSelector, findOptions),
                function() {
                  resolve(Meteor.users.findFromPublication('user-list-admin', {}, findSortOption).fetch())
                  handle.stop()
                }
              )
            })
          }
    }

Perhaps a cleaned up version of this example can be put into the documentation.

Tallyb commented 8 years ago

A similar example actually exists in the Angular-Meteor 1.3 blog post: http://info.meteor.com/blog/angular-meteor-1.3 (see the subscribe section)

However, the issue I saw with this is that the subscription cannot be stopped from the controller, as the handler does not return. Are you stopping the subscription right away?

trajano commented 8 years ago

That's correct, because the subscription is not reactive on the resolve. I have another subscription that is on the controller that would pass the subscribe arguments with $reactive attached on $scope so the list is still reactive and I presume will get stopped when the $scope goes out of scope which normally happens when ui-router state changes.

BTW thanks for the link, I didn't know that page existed. However, I did notice a few things that I did differently.

trajano commented 8 years ago

So just added the error handling, though I don't have any test case for this yet so I am not sure if it does work on error. I do know that the onStop does get called when I do handle.stop so I added a variable to check whether it has been resolved first and guard against it before rejecting.

  resolve: {
    ... findSelector, findSortOption and findOptions resolvers ...
    initialDisplayedUsers: function($q, findSelector, findOptions, findSortOption) {
      return $q((resolve, reject) => {
        var resolved = false
        var handle = Meteor.subscribe('user-list-admin', findSelector, findOptions),
                {
                  onReady: _ => {
                    resolve(Meteor.users.findFromPublication('user-list-admin', {},sortOptions).fetch())
                    resolved = true
                    handle.stop()
                  },
                  onStop: _ => (resolved || reject())
                }
              )
            })
          }
    }

Perhaps a cleaned up version of this example can be put into the documentation.

sean-stanley commented 8 years ago

It might be noted that best practice with React and Blaze now is to not have your subscriptions happen before your route loads (like in Iron Router) a better approach according to the people at Kadira is to load your route with only minimal subscription logic and then use your controller (or onCreated function in Blaze) to create the subscription reactively. The idea is that while your route/component loads it's data use spinners and such and display the data when the subscription finishes. In Blaze you can just use the helper {{Template.SubscriptionsReady}} in Angular JS I do ng-if="$ctrl.yourHelperUsingCollectionYouSubscribedToo.length > 0"

trajano commented 8 years ago

@sean-stanley the problem with that approach is if the data set is large say 100 records or in my case 10 records on a slow machine I see the table being rendered as some records are being retrieved. It's more distracting than having the page get displayed once all the data is resolved.

I still have the table re-rendering issue when I sort the tables because I don't change state and the subscription gets updated reactively.

sean-stanley commented 8 years ago

Another way then is in your controller where you start the subscription, add an onReady callback.

this.subscribe('someSubscriptionWithLotsOfDocuments',  () => {//get array of arguments reactively}, () => {
  // on Ready
  this.subscriptionReady = true
});

Then in your template use:

ng-if="$ctrl.subscriptionReady"

Then you won't have pieces of your subscription being rendered in chunks.

Unfortunately, ng-repeat seems to be way weaker than React and Blaze's own {{each}} in terms of efficiently creating and watching DOM nodes. Large collections in ng-repeat really bog down an application quite quickly.

trajano commented 8 years ago

@sean-stanley interesting approach, I am assuming subscribe does an automatic $scope.$apply on the callback. Though this approach from what I can garner would give me a "empty table" until it is ready. However, I think the UX would be better if the view does not load and show a loading animation until everything I need is ready (I have a used https://github.com/angular-ui/ui-router/wiki#view-load-events to put a loading animation while the states are being transition)

sean-stanley commented 8 years ago

I see where you are coming from and using generic view loading code would be more convenient however, I like the little bit more control of my own approach. You can see it in action at http://new.foodcoop.nz Rather than have the entire view display a loading symbol only my catalog has a spinner, this means, the categories, search bar, sort menu and more can all be rendered when they are asynchronously ready. I'm not sure it hurts the UX this approach and I am open to suggestions to improve on my own implementation.

kamilkisiela commented 8 years ago

@trajano I think you can just use Meteor.subscribe with $q.

resolve: {
  books() {
    var deferred = $q.defer();
    const handle = Meteor.subscribe('chats', {
      onReady: () => deferred.resolve(handle),
      onStop: deferred.reject
    });

    return deferred.promise;
  }
}

And then:

controller(books) {
  books.stop();
}
kamilkisiela commented 8 years ago

Let me see how it works for you so we can close this issue :)

trajano commented 8 years ago

@kamilkisiela problem with that approach is the controller becomes responsible for stopping. Where mine stops it on the resolve block

kamilkisiela commented 8 years ago

@trajano It's just another solution, depends on what you want to achieve :)

You asked about replacement for $meteor.subscribe so here it is :)

If you need reactivity then stopping subscription inside resolve block is not a good idea.

If you want to just fetch data then it's okay to use it your way.

trajano commented 8 years ago

I checked my code now, I just took it out since I had issues with reactivity (though it was for some other reason). As long as the API documentation for meteor subscribe on the angular-meteor gets updated with a sample I think we should be fine to close this.

sean-stanley commented 8 years ago

Just a quick thought, if you want to receive a bunch of data from the server as a one-off than a subscribe call is really not the best way to do that. Your best off writing a meteor method that does that and returns the cursor.fetch() results. Then you don't have any reactive overhead for your static call.

Unfortunately Meteor.call isn't available as a promise anymore either though as @Urigo pointed out in a blog post, if people want to use Meteor.call and Meteor.subscribe as promises, it should be added to the core API so all frameworks (blaze, react, angular) can use it. Wrapping Meteor.call in promises would work the same as @kamilkisiela solution.

sean-stanley commented 8 years ago

Not trying to criticize or anything but there may be alternative ways of doing whatever it is you want the promises for.

I've read a lot about trends in modern component driven apps. They all want to remove as much as possible data logic from the router. They have some good points on why to do this. So maybe in your case, instead the controller should subscribe and fetch the data it needs and display a loading icon or similar while it fetches the data. For static data, angular components have hooks in their controllers you can use like $onInit().

If you want to read more about this way of handling data here are some links.

Angular JS Components guide Meteor Routing Guide on Data and Subscription Management

trajano commented 8 years ago

I have a while loading event on the template that would be running while data is being loaded in the resolve events for the router.

kamilkisiela commented 8 years ago

If someone needs Promises for Meteor.subscribe(), Meteor.call() or Meteor.apply() you can use angular-meteor-promiser.

Tallyb commented 8 years ago

@kamilkisiela I have not tried it yet, but you are now officially my hero!

kamilkisiela commented 8 years ago

Oh, stop it, @Tallyb! It's just a simple wrapper function :) I made it so everyone can maintain the same repository instead of creating their own services.

Urigo commented 8 years ago

yes!

trajano commented 8 years ago

I still do not see the document http://www.angular-meteor.com/api/1.2.2/subscribe updated with "old code" vs "new code"

Urigo commented 8 years ago

@trajano you are right, could you try to PR the docs?

trajano commented 8 years ago

@kamilkisiela now your wrapper package is there for prosperity.