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

Update index.jsx #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Update index.jsx #13

wants to merge 2 commits into from

Conversation

Elviron
Copy link

@Elviron Elviron commented Jun 6, 2016

Save the subscriptions based on only the name of the subscription.
So you don't save multiple subscriptions of the same name.

Save the subscriptions based on only the name of the subscription.
So you don't save multiple subscriptions of the same name.
@maxnowack
Copy link

With this, it isn't possible to have multiple subscriptions to the same publication in the same component. I think it's better to wrap the subscribe call in a autorun, since subscription inside autoruns will automatically stopped if the autorun will rerun.

@timbrandin
Copy link
Member

Great idea @maxnowack, that would remove the need to track the subscriptions on each component, and reduce the load on GC too. So the implementation would rather be:

this.autorun(() => this.__subscribe.apply(this, [name, ...options]))

Rather than the current lookup and removal. What do you think @Elviron ?

@maxnowack
Copy link

@timbrandin I don't think that it is a good idea to wrap every subscribe in an autorun. If you take a look at the implementation in blaze, you'll see that the subscription handle will be saved as a reference and the subscription will be stopped if the view gets destroyed (Tracker.Component does something similar with tracker computations). In my opinion, this is the most flexible way.
I could provide a PR for this.

PS: I don't get why you stringify the arguments of the subscriptions to store the reference. Is there a reason for this?

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

Successfully merging this pull request may close these issues.

3 participants