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

Template-level subscriptions #3559

Closed
wants to merge 2 commits into from
Closed

Template-level subscriptions #3559

wants to merge 2 commits into from

Conversation

@stubailo
Copy link
Contributor

@stubailo stubailo commented Jan 26, 2015

Now, you can call this.subscribe inside the onCreated callback of a Template. This subscription is automatically stopped when the template is destroyed. You also get a bonus built-in helper, Template.ready, that returns true when all of the subscriptions are ready. You can also manage your ready state independently by using the handles returned from this.subscribe.

@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Jan 26, 2015

Added tests and documentation for the new feature.

@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Jan 27, 2015

@avital @dgreensp review please?


tmpl.onCreated(function () {
subHandle = this.subscribe("items", subscribeCallback);
subHandle.stop = stopCallback;

This comment has been minimized.

@avital

avital Jan 27, 2015
Contributor

That's a nice idea -- basically mock out the stop method instead of testing for the results of it being called. I'll use this in other tests!

});

tmpl.onCreated(function () {
subHandle = this.subscribe("items", subscribeCallback);

This comment has been minimized.

@avital

avital Jan 27, 2015
Contributor

Maybe have more than one subscription? So that you test the behavior of "they all get stopped", and the behavior of "only if they are all ready is Template.ready true"

This comment has been minimized.

@stubailo

stubailo Jan 28, 2015
Author Contributor

Added a test for multiple subs!

// Manually check the subscribe ready callback to make sure the template is
// doing the right thing
var subscribeCallback = expect(function () {
Tracker.afterFlush(checkHTMLThenRemove);

This comment has been minimized.

@avital

avital Jan 27, 2015
Contributor

I think you can simplify this by instead calling Tracker.flush(). Which would get rid of one layer of expect and function definitions.

This comment has been minimized.

@stubailo

stubailo Jan 27, 2015
Author Contributor

For some reason, it doesn't work if I do that.

This comment has been minimized.

@dgreensp

dgreensp Jan 27, 2015
Contributor

Maybe subscribe is using afterFlush itself.

var self = this;

if (! self.isCreated) {
throw new Error("View#subscribe must be called from the created callback at the earliest");

This comment has been minimized.

@avital

avital Jan 27, 2015
Contributor

Are there additional limitations? Like -- a user probably shouldn't call this from the destroyed callback?

And if it's OK to call this from the rendered callback I think it would be good to test that explicitly (in the same test if needed, just add an additional subscription from a rendered callback)

if (! self.isCreated) {
throw new Error("View#subscribe must be called from the created callback at the earliest");
}
if (this._isInRender) {

This comment has been minimized.

@avital

avital Jan 27, 2015
Contributor

If I understand correctly, these two if clauses (and the one above I guess) are really an attempt to say: "if not in a created or rendered callback throw an error". Is there a better way to check for this? (I don't know the answer). It looks a little ad-hoc.

At the very least I'd recommend extracting these into a utility function called, say, "canCallSubscribe" that's documented as checking if you're in a created or rendered callback with a comment saying this is the best way you found to check for this.

Maybe @dgreensp has a better suggestion.

This comment has been minimized.

@dgreensp

dgreensp Jan 27, 2015
Contributor

This is a typical pattern in the Blaze code, but maybe it should be moved into a helper.

This comment has been minimized.

@stubailo

stubailo Jan 27, 2015
Author Contributor

Yeah I copy-pasted this from this.autorun :]

Definitely should be refactored a little

@avital
Copy link
Contributor

@avital avital commented Jan 27, 2015

Added some comments, but this looks great! So simple!

@Slava Slava added the Blaze label Jan 27, 2015
@arunoda
Copy link

@arunoda arunoda commented Jan 27, 2015

Very nice :)

'dynamic': 'Template.__dynamic'
'dynamic': 'Template.__dynamic',

'ready': 'view.templateInstance().ready()'

This comment has been minimized.

@dgreensp

dgreensp Jan 27, 2015
Contributor

If view isn't a template, this will throw an error that view.templateInstance is undefined, right? Shouldn't we throw a better error?

This comment has been minimized.

@stubailo

stubailo Jan 27, 2015
Author Contributor

I think that is impossible because this code can only occur inside of a template.

This comment has been minimized.

@dgreensp

dgreensp Jan 28, 2015
Contributor

Oh, you're right, view is actually the template's view.

* @return {Boolean} True if all subscriptions on this template instance are
* ready.
*/
Blaze.TemplateInstance.prototype.ready = function () {

This comment has been minimized.

@dgreensp

dgreensp Jan 27, 2015
Contributor

This could break code that uses a ready property on the template instance. It doesn't break the Discover Meteor code, which sets this.ready = new ReactiveVar on the template instance (because the inherited property will be shadowed), but it would break someone's code who checks this.ready but never explicitly initializes it to false (since now this.ready starts out truthy).

I think I'm ok with that, but we should at least have a unit test that verifies that shadowing this.ready() works. (I don't see any tests of this.ready() actually, only Template.ready.) That way, we won't accidentally break apps that rely on being able to set this.ready (like apps following Discover Meteor's pattern) at some point in the future if we change how the template instance object works.

If someone shadows this.ready, I also note that it will break Template.ready. But we could decide we don't care about that.

This comment has been minimized.

@dgreensp

dgreensp Jan 27, 2015
Contributor

And we should mention it wherever we mention breaking changes.

It could turn out that a lot of people set this.ready in their apps and are unclear on whether they need to change that code now or not.

This comment has been minimized.

@stubailo

stubailo Feb 11, 2015
Author Contributor

After talking to matt, will change this to subscriptionsReady

@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Jan 28, 2015

Currently in the middle of implementing onStopped for subscriptions, put this CR on hold please

}

if (subscriptionsFinished === 2) {
Tracker.afterFlush(checkBothReady);

This comment has been minimized.

@avital

avital Jan 28, 2015
Contributor

(Lost the original comment since you re-wrote the test)

Before we merge this, I'd like to understand why we can't use Deps.flush. (Happy to pair on it if that would be helpful)

This comment has been minimized.

@stubailo

stubailo Feb 11, 2015
Author Contributor

No progress on this. Do you want to pair on it?

@Tarang
Copy link
Contributor

@Tarang Tarang commented Jan 30, 2015

Is it not possible to allow a reactive change with this, especially when the Tracker computation is not necessarily 'on its own' but part of a the template's lifetime too?

i.e

Template.hello.onCreated(function() {
    var self = this;
    self.autorun(function() {
      self.subscribe("list", Session.get('category'))
    });
  })
@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Jan 30, 2015

@Tarang the intention is that the code you provided will work as expected. I should add a test for that particular use case.

@stubailo stubailo force-pushed the template-subs branch Feb 11, 2015
@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Feb 11, 2015

Rebased on devel and fixed minor merge conflicts. @glasser, what do you think?

In particular, I have made two changes to subscriptions:

  1. The onError callback is deprecated now, and replaced with onStopped with an optional error callback. I still have to write docs for this change, but it's fully backwards compatible.
  2. Meteor.subscribe inside an autorun always returns the exact same handle, so you can compare them with ===.
@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Feb 11, 2015

OK all of the tests pass and I have added a lot of new tests. @glasser, @avital what else should I do before merging this? Also, should I rebase some things to reduce the number of commits?

@glasser
glasser reviewed Feb 11, 2015
View changes
docs/client/full-api/api/pubsub.md Outdated
The `onReady` callback is called with no arguments when the server [marks the
subscription as ready](#publish_ready). The `onStopped` callback is called with
a [`Meteor.Error`](#meteor_error) if the subscription fails or is terminated by
the server. If the subscription is stopped for any other reason, `onStopped` is

This comment has been minimized.

@glasser

glasser Feb 11, 2015
Member

we can be specific --- stopped by calling stop on either the client or the server (right?)

are there other ways? hmm, i actually have no idea what happens when you close a DDP connection (with the not-really-documented connection.close() --- does it stop the subscriptions and remove their contents from minimongo? guessing not, though that is actually kinda weird.

This comment has been minimized.

@glasser

glasser Feb 11, 2015
Member

also i'm not sure if we should still document onError too (as deprecated). I guess we usually don't. but it does make it harder for people to read slightly old code. well, as long as History.md is updated....

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

I don't think we have a history of documenting deprecated things. There's a general issue here where it's really hard to access the docs of old meteor versions. Maybe it's worth uploading all of the old docs apps with URLs like "docs-1.0.3.1.meteor.com" or something

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

Ah yes I guess I meant --- What you are doing here is totally the way we do things here. I'm starting to think it's not ideal for users, though.

It wouldn't actually be all that hard to upload all the old docs apps...

@glasser
glasser reviewed Feb 11, 2015
View changes
packages/ddp/livedata_connection.js Outdated
self._subscriptions[msg.id].remove();

var meteorErrorFromMsg = function (msgArg) {
return msgArg && new Meteor.Error(

This comment has been minimized.

@glasser

glasser Feb 11, 2015
Member

i feel like this is missing a check on msgArg.error

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

Done!

@@ -267,6 +267,8 @@ Blaze.TemplateInstance = function (view) {
* @type {DOMNode}

This comment has been minimized.

@glasser

glasser Feb 11, 2015
Member

Is it possible to separate this into one commit which adds the feature to ddp (and doesn't touch blaze at all) followed by one commit that does the blaze/spacebars stuff?

eg, let's say we find a bug in the blaze thing and revert it. would be a shame to lose the new feature.

if you need help splitting git commits in two i can help :)

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

This is the commit you want to split? 1f77d8e

@glasser
glasser reviewed Feb 12, 2015
View changes
packages/ddp/livedata_connection.js Outdated
existing.stoppedCallback = callbacks.onStopped;
}

// Return the exact same handle so that we can use === to check if the

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

interesting. where are you using this? this seems like a strong assumption that, if we want to make it dependable, should be documented.

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

In order to stop all of the subscriptions when the template is destroyed, I need to keep a list of all of the handles. To stop that list from growing without bound, I need to be able to deduplicate them. My two options were either using === to compare handles or having the handle include the subscription id. I thought not leaking the ID was the less risky option.

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

That makes sense, but in that case it should probably be documented, at least internally. At the very least, the (good) comment that you added here could have a link to the blaze code that cares. Or it could even be in the real docs (which does describe the de-duplication), though that links would still be good.

On the other hand --- the id is an actual real part of the DDP wire protocol, not an internal implementation detail of the ddp package, and arguably we should just expose it on the handle?

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

I'm open to either one. It seems weird to generate lots of identical handles on the one hand, on the other hand the id solution could be less magical and easier to document. Maybe we should do both? Basically don't generate lots of unnecessary handles as an optimization but also put the id on there, document that change, and use it everywhere.

What do you think?

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

On the other hand it will make code like _.union(subHandles.get(), [subHandle]) less nice to start using the subscriptionId.

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

well, i mean, just use a map instead of an array? then it actually becomes easier? (or even ReactiveDict instead of ReactiveVar if that makes sense)

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

(though you can't iterate over a reactivedict... see #3135)

@glasser
glasser reviewed Feb 12, 2015
View changes
packages/ddp/livedata_tests.js Outdated
@@ -601,29 +611,109 @@ if (Meteor.isClient) {
}
];})());

testAsyncMulti("livedata - publish multiple cursors", [
testAsyncMulti("livedata - publisher errors with onStopped callback", (function () {

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

copy and pasting the whole test is not really ideal (though the ugly thing we do where we make tests that run in multiple guises isn't super great either) but better than no test

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

I guess I could do a for loop and define some function like "onStoppedPretendingToBeOnError" but this seemed easier to understand.

@glasser
Copy link
Member

@glasser glasser commented Feb 12, 2015

I didn't look at the blaze side of things, but finished a pass on the ddp side

* @return {Boolean} True if all subscriptions on this template instance are
* ready.
*/
Blaze.TemplateInstance.prototype.subscriptionsReady = function () {

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

It seems like this will often trigger deps changed without a value change. eg, on unsub, it will stay true.

what's the function that you can use to make that not happen?

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

Fixed this

@glasser
glasser reviewed Feb 12, 2015
View changes
packages/blaze/view.js Outdated

self._errorIfShouldntCallSubscribe();

var subHandle = Meteor.subscribe.apply(Meteor, arguments);

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

would be nice, in theory, to have a way to use a different connection than the default connection. Not sure how you would work it into this varargs API. I guess you could just have a different function subscribeOnConnection

This comment has been minimized.

@stubailo

stubailo Feb 12, 2015
Author Contributor

I think we can leave that for a later time until people ask for it?

This comment has been minimized.

@glasser

glasser Feb 12, 2015
Member

Sure, as long as you expect that the answer is going to be something like "add another function with a different name" and not "I sure wish I had designed this function to take an options argument but I can't now".

The obvious case to me is — are we going to want to use this behavior in accounts-ui? Because that's going to want to use Accounts.connection.

@stubailo stubailo force-pushed the template-subs branch 2 times, most recently Feb 12, 2015
@stubailo stubailo force-pushed the template-subs branch 2 times, most recently Feb 12, 2015
@stubailo stubailo force-pushed the template-subs branch to 5017524 Feb 12, 2015
@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Feb 12, 2015

Merged manually

@stubailo stubailo closed this Feb 12, 2015
@KrishnaPG
Copy link

@KrishnaPG KrishnaPG commented Apr 10, 2015

Looks interesting and would love to use this kind of thing - any examples / tutorials on the how to take advantage of this feature?

@stubailo
Copy link
Contributor Author

@stubailo stubailo commented Apr 10, 2015

Here are the official docs: http://docs.meteor.com/#/full/Blaze-TemplateInstance-subscribe

Here is a great article by discover Meteor, written before the feature was added to Meteor: https://www.discovermeteor.com/blog/template-level-subscriptions/

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

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.