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

onStop callback for Meteor.subscribe #1461

Closed
mitar opened this issue Oct 1, 2013 · 21 comments
Closed

onStop callback for Meteor.subscribe #1461

mitar opened this issue Oct 1, 2013 · 21 comments

Comments

@mitar
Copy link
Collaborator

@mitar mitar commented Oct 1, 2013

Would it be possible to provide onStop callback for Meteor.subscribe? So that some additional cleanup can be done afterwards? In this way it is easier to have logic chained together: you first subscribe and do other initialization stuff, and when you move to another page, you just kill subscription and the rest will follow? I agree that many things can be done without it, but it seems to me that it opens sometimes a path to do some things easier.

@mitar
Copy link
Collaborator Author

@mitar mitar commented Oct 2, 2013

For example, calling .stop() does not call onError on the client. So something else should be available on the client.

@glasser
Copy link
Member

@glasser glasser commented Oct 9, 2013

My initial reaction was "why don't you just call your callback when you call stop()", but I'm guessing the point is that you're using autorun and not explicitly calling stop. On further thought, this does seem reasonable.

Design questions include: should it be called in addition to onError, or should it be specific to client-initiated stops? I'd lean towards the latter. Also, should we be careful to call it outside of the current Deps context (since it is most likely to be called as part of an autorun processing)?

@mitar
Copy link
Collaborator Author

@mitar mitar commented Oct 10, 2013

In fact, my idea was to get a callback on the client, when the server stops the subscription. For Deps.autorun you can probably just use Deps.onInvalidate or Deps.afterFlush, no? It is not exactly the same as onStop would be, so onStop is useful in Deps.autorun as well (especially because subscription might not be really stopped on all Deps.onInvalidate events, if they resubscribe to the same thing).

should it be called in addition to onError, or should it be specific to client-initiated stops?

So no, it should be useful especially for server initiated stops. So if subscription is stopped by the server before autorun is invalidated, client should be able to do something?

Also, should we be careful to call it outside of the current Deps context (since it is most likely to be called as part of an autorun processing)?

Why would it be useful to run outside Deps context? I would do what is the most expected. Because you are passing callback inside the Deps.autoron you are probably expecting to be in that context? It would be probably useful that you could do Deps.currentComputation.invalidate() inside the onStop of subscription inside the Deps.autorun, no?

@glasser
Copy link
Member

@glasser glasser commented Oct 10, 2013

onError does run for server side stops, I thought.

@mitar
Copy link
Collaborator Author

@mitar mitar commented Oct 10, 2013

Not when I tested it?

@mitar
Copy link
Collaborator Author

@mitar mitar commented Oct 10, 2013

I tried:

if (Meteor.isClient) {
  Deps.autorun(function () {
    Meteor.subscribe("rooms", {
      onReady: function () { console.log("onReady", arguments); },
      onError: function () { console.log("onError", arguments); }
    });
  });
}

if (Meteor.isServer) {
  Meteor.publish("rooms", function () {
    this.ready();
    this.stop();
  });
}

And I get only onReady in the console.

@mitar
Copy link
Collaborator Author

@mitar mitar commented Oct 10, 2013

In any case, calling onError when there is no error is not really a good API. Maybe we could have onStop only and then first argument could be error, and the rest could be other optional arguments (#1480).

@glasser
Copy link
Member

@glasser glasser commented Oct 10, 2013

Oh, sorry, you're completely right about onError.

On Wed, Oct 9, 2013 at 7:19 PM, Mitar notifications@github.com wrote:

In any case, calling onError when there is no error is not really a good
API. Maybe we could have onStop only and then first argument could be
error, and the rest could be other optional arguments (#1480#1480
).


Reply to this email directly or view it on GitHubhttps://github.com//issues/1461#issuecomment-26024459
.

@glasser
Copy link
Member

@glasser glasser commented Oct 18, 2013

@mitar suggested in #1480 that if onStop is implemented, it perhaps should be able to receive some data that is passed as an argument to stop.

That said, maybe you should just use error/onError in this case...

@mitar
Copy link
Collaborator Author

@mitar mitar commented Oct 18, 2013

I think it is a different semantic purpose. I feel like passing a valid return value should not be mixed with error return value. Maybe you want for errors some global handling, but for values local handling.

@bline
Copy link

@bline bline commented Oct 21, 2013

I'm currently working with observeChanges on the client and filling out a search results table with results of what was typed in a search box. the search actually changes the arguments to the subscription. It seems when that happens, Meteor ends the current subscription and starts a new one. The observeChanges first gets added callbacks for each item from the new subscription, then it gets all the removed callbacks from the last subscription. Sometimes a few seconds later depending on how large the result of the new subscription is.

I think this behavior is fine, but it would be nice to have some loading indicator so the user isn't confused by seeing results from the last search for several seconds while the new subscription is populating and the old subscription is clearing out.

it would be nice to provide some kind of method to find out when the old subscription is actually stopped, i.e. all it's items have been cleared out of the local collection. Because the subscriptions are the same one from the point of view of my code, only the arguments have changes, I have no way now to do this transition.

I've looked through the actual server messages that are coming across the wire and it seems there is a nosub message from the previous subscription which always fires once all the remove messages have come across the wire, but I see no way to tie into this in the Meteor code handling the connection. From what I can see it's only used for error handling and if the unsubscribe was stopped by the current client process, the message is just dropped.

Any thoughts on some way to tie into this in the client code so we can do smoother transitions from one subscription to the next?

@bline
Copy link

@bline bline commented Oct 21, 2013

Forgot to include a link to the post I made on meteor-talk giving more detail on what was observed from the current behavior: https://groups.google.com/forum/#!topic/meteor-talk/KvcvUY9oBgg

@mitar
Copy link
Collaborator Author

@mitar mitar commented Aug 21, 2014

Yes, there are two things missing. onStop argument which you would pass at subscription time. But also a callback to .stop() which you can pass when you are calling stop on subscription handle. For example, in testAsyncMulti tests I cannot call expect like:

subscriptionHandle.stop(expect())

and then wait before the next block of tests that subscription is really stopped and all documents removed.

@mitar
Copy link
Collaborator Author

@mitar mitar commented Aug 21, 2014

In other words, how to get a callback when nosub message arrives?

@mitar
Copy link
Collaborator Author

@mitar mitar commented Feb 12, 2015

Amazing!

@stubailo
Copy link
Contributor

@stubailo stubailo commented Feb 12, 2015

Hehe such a fast response time.

@steph643
Copy link

@steph643 steph643 commented Mar 2, 2015

In other words, how to get a callback when nosub message arrives?

@mitar, did you find a way to achieve that?

@mitar
Copy link
Collaborator Author

@mitar mitar commented Mar 2, 2015

In next release of Meteor, there will be this onStopped callback mentioned in 085e02d.

@steph643
Copy link

@steph643 steph643 commented Mar 2, 2015

Thanks @mitar.

There is also a discussion on this here.

@erperejildo
Copy link

@erperejildo erperejildo commented Mar 28, 2017

Testing this but I can get this working. Created issue here: Urigo/angular-meteor#1584
Resuming: my callback doesn't work. Checked with setTimeout again the ddbb and it works

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.