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

Subscription onReady callbacks don't fire on a re-subscription with the same arguments inside Deps.autorun #1173

Closed
athiwatc opened this Issue Jun 21, 2013 · 28 comments

Comments

Projects
None yet
@athiwatc

athiwatc commented Jun 21, 2013

_2 Upvotes_ I don't know if this is the intended behavior but lets assume that the server publish is setup.

Then we do

Deps.autorun ->
  Session.set('ready', false)
  Meteor.subscribe 'call-this', Session.get('something'), ->
    Session.set('ready', true)

Session.set('something', 2)
#wait for the subscibe to complete and the page shows
Session.set('something', 2)
#Now the loading page gets stuck. Note that the loading page uses {{#if}} against the 'ready' session.

I don't really have time to get an example working right now but I will try to if I get time.

My current hack/fix right now is adding a non-sense field to trick meteor on tearing and setting up the subscription and calling the on finish again.

Deps.autorun ->
  Session.set('ready', false)
  Meteor.subscribe 'call-this', Session.get('something'), Math.random(), ->
    Session.set('ready', true)

Session.set('something', 2)
#wait for the subscibe to complete and the page shows
Session.set('something', 2)
#Now the loading page gets stuck. Note that the loading page uses {{#if}} against the 'ready' session.

@athiwatc athiwatc closed this Jun 27, 2013

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 13, 2014

I just ran into this. I think it's a bug. Meteor.subscribe should fire the onReady callback immediately if the re-subscribe inside Deps.autorun has the same arguments and the server isn't actually getting hit again. It currently only fires on the initial subscription, so on a router-triggered re-render my page gets stuck on loading spinners even though the data is ready.

@glasser: this bug is simple enough to understand, but do you want me to create a demo repo? I was just about to report it.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 27, 2014

Anyone, can we re-open this issue please? I will happily create a repro for it.

@estark37

This comment has been minimized.

Contributor

estark37 commented Aug 30, 2014

Reopening - a repro would be appreciated, @mizzao.

@estark37 estark37 reopened this Aug 30, 2014

@mizzao

This comment has been minimized.

Contributor

mizzao commented Aug 30, 2014

The title should be changed to

Subscription onReady callbacks don't fire on a re-subscription with the same arguments inside Deps.autorun

I'll create a repro pronto.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Sep 3, 2014

Hi @estark37, reproduction is here: https://github.com/mizzao/publish-autorun-onready.

When page loads, onReady is called for the initial subscription. When the subscription happens again with the same arguments, it is not actually sent to the server (as documented in http://docs.meteor.com/#meteor_subscribe) but the onReady callback is not called.

When the latter happens, onReady should be called immediately after the re-subscribe.

@glasser glasser changed the title from Deps.autorun and Meteor.subscribe is too smart or is it suppose to be this way. to Subscription onReady callbacks don't fire on a re-subscription with the same arguments inside Deps.autorun Sep 5, 2014

@glasser

This comment has been minimized.

Member

glasser commented Sep 5, 2014

Yeah, that sounds reasonable. This falls into the case that we mention in a comment in livedata_connection.js:

        // If the sub is not already ready, replace any ready callback with the
        // one provided now. (It's not really clear what users would expect for
        // an onReady callback inside an autorun; the semantics we provide is
        // that at the time the sub first becomes ready, we call the last
        // onReady callback provided, if any.)

Perhaps it is now clear what users would expect!

I think the change is just to replace

        if (!existing.ready)
          existing.readyCallback = callbacks.onReady;

with

        if (existing.ready) {
          callbacks.onReady();
        } else {
          existing.readyCallback = callbacks.onReady;
        }

We would review a PR that made this change, updated the nearby comments appropriate, and added a test.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 6, 2014

Just linking one for me related ticket: #1461, so I think we should have better ways to get callbacks when subscription stop, and how they are restarted.

Because it is not clear how this should behave. In some other situations you do not want onReady to be triggered if subscription didn't really close and come back. Maybe we need onReady, onRerun, onClose. onRerun would be called only if nothing changed, but it was rerun, onReady would be called for each new subscription.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 6, 2014

So onReady, onRerun, onClose, onError.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 7, 2014

Is this related to #2440? I think it maybe should be addressed together?

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 7, 2014

I looked in our code how we are doing it and found that we do this differently and this is why I have not encountered this issue:

var groupHandle = null;
var groupSubscribing = new Variable(false);

Deps.autorun(function() {
  if (Session.get('currentGroupId')) {
    groupSubscribing.set(true);
    groupHandle = Meteor.subscribe('groups-by-ids', Session.get('currentGroupId'));
  } else {
    groupSubscribing.set(false);
    groupHandle = null;
  }
});

Deps.autorun(function() {
  if (groupSubscribing() && groupHandle && groupHandle.ready()) {
    groupSubscribing.set(false);
  }
});

Template.group.loading = function() {
  groupSubscribing(); // To register dependency
  return !(groupHandle && groupHandle.ready());
};

Template.group.notFound = function() {
  groupSubscribing(); // To register dependency
  return (groupHandle && groupHandle.ready() && !Group.documents.findOne(Session.get('currentGroupId'), {
    fields: {
      _id: 1
    }
  });
};

Template.group.group = function() {
  return Group.documents.findOne(Session.get('currentGroupId'));
};

I think this is quite ugly and probably Meteor should provide some nicer way to do it.

@mitar

This comment has been minimized.

Collaborator

mitar commented Sep 7, 2014

(Using meteor-variable there, instead of Session.)

@mizzao

This comment has been minimized.

Contributor

mizzao commented Sep 8, 2014

It doesn't matter if you use Meteor.Variable or Session, you are doing it differently by checking the reactive subHandle.ready() function rather than waiting for the onReady callback.

It seems like the first works as expected in this case. These two should do the same thing, hence why this fix is required.

As @tmeasday said, I think this is different from #2440.

@mizzao

This comment has been minimized.

Contributor

mizzao commented Mar 11, 2015

Is a fix for this going to make it into 1.0.4 along with the other subscription-related updates?

@glasser

This comment has been minimized.

Member

glasser commented Mar 11, 2015

No, 1.0.4 is currently in release candidate stage and there hasn't been work on this issue.

@thehecht

This comment has been minimized.

thehecht commented Apr 3, 2015

Is this still considered a bug or is intended behaviour? My team adopted the current behaviour as it is using the reactive subhandle.ready() whenever we want to ensure that the data is ready but using the onReady callback when we need to know that fresh data just came from the server. If it's a bug we should not use the second case.

@RobertLowe

This comment has been minimized.

Contributor

RobertLowe commented Jun 22, 2015

+1 this issue is severely annoying

@stubailo stubailo added the triaged label Oct 22, 2015

@kokokenada

This comment has been minimized.

kokokenada commented Mar 16, 2016

+1

@zol zol removed the backlog label May 3, 2016

@kooshywoosh

This comment has been minimized.

kooshywoosh commented Jul 15, 2016

+1

@DriesVandermeulen

This comment has been minimized.

DriesVandermeulen commented Jul 28, 2016

+1 still need this

@reason211

This comment has been minimized.

reason211 commented Aug 27, 2016

+10086

@jetlej

This comment has been minimized.

jetlej commented Sep 14, 2016

+1

@Taxel

This comment has been minimized.

Taxel commented Nov 8, 2016

+1

@erperejildo

This comment has been minimized.

erperejildo commented Mar 28, 2017

This issue almost has 4 years. Any solutions?
+1

@felipenmoura

This comment has been minimized.

felipenmoura commented Apr 6, 2017

+Number.MAX_SAFE_INTEGER

@danielparas

This comment has been minimized.

danielparas commented Apr 30, 2017

+1

zimme added a commit to zimme/meteor that referenced this issue Jun 2, 2017

@benjamn benjamn closed this in e51e72b Jul 12, 2017

@RobertLowe

This comment has been minimized.

Contributor

RobertLowe commented Jul 13, 2017

4 years wow, time flies and Meteor still improves!

@benjamn while this doesn't need backwards compatibility per-say I think it's going to cause some strange behaviours for apps not expecting this (and having the callback suddenly re-fire after an app update), any thoughts here?

👍

@hwillson

This comment has been minimized.

Member

hwillson commented Jul 13, 2017

@RobertLowe This was discussed in the original PR (#8754). It was decided that the fact that this could be a potentially breaking change would be documented in the History.md (5th bullet). Correcting this buggy behavior was deemed worth the potential BC headache.

@RobertLowe

This comment has been minimized.

Contributor

RobertLowe commented Jul 13, 2017

@hwillson Thanks, glad to see it was considered

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