Create convenience mixin to manage subscriptions #7

Closed
bripkens opened this Issue Jul 28, 2014 · 6 comments

Comments

Projects
None yet
2 participants
@bripkens
Contributor

bripkens commented Jul 28, 2014

Each component needs to unsubscribe to avoid havoc. Having a component subscribe to multiple listenables only complicates the situation.

I propose to add a mixin which manages the subscriptions and takes care of unsubscribing. It may look similar to the following:

var ListenerMixin = {
  componentWillMount: function() {
    this.subscriptions = [];
  },

  listenTo: function(listenable, callback) {
    var unsubscribe = listenable.listen(callback, this);
    this.subscriptions.push(unsubscribe);
  },

  componentWillUnmount: function() {
    this.subscriptions.forEach(function(unsubscribe) {
      unsubscribe();
    });
  }
}
@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Jul 28, 2014

Member

It is a nice idea.

I'm not really sure how to use the mixin in the cases where you will need to override the component's lifecycle methods (i.e. the componentWillMount and componentWillUnmount) for other uses than handling subscription. Any suggestions for this?

Member

spoike commented Jul 28, 2014

It is a nice idea.

I'm not really sure how to use the mixin in the cases where you will need to override the component's lifecycle methods (i.e. the componentWillMount and componentWillUnmount) for other uses than handling subscription. Any suggestions for this?

@spoike spoike added the enhancement label Jul 28, 2014

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Jul 28, 2014

Member

I think we can though, with an API that looks like this:

var ctx = Reflux.mixinClass({
  componentWillMount: function() {
    // overrides the mixin's componentWillMount
  },
  componentWillUnmount: function() {
    // overrides the mixin's componentWillUnmount
  }
});

var ReactComponent = React.createClass(ctx);

So when Reflux.mixinClass encounters componentWillMount or componentWillUnmount in the ctx, then it'll do something like this that keeps the implementation from the mixin:

// e.g. componentWillMount

// mixin as usual here

if (ctx.componentWillMount) {
  outputClass.componentWillMount = function() {
    ctx.componentWillMount();
    ListenerMixin.componentWillMount();
  }
}

Obviously have to figure out how to fix the binding context.

Thoughts?

Member

spoike commented Jul 28, 2014

I think we can though, with an API that looks like this:

var ctx = Reflux.mixinClass({
  componentWillMount: function() {
    // overrides the mixin's componentWillMount
  },
  componentWillUnmount: function() {
    // overrides the mixin's componentWillUnmount
  }
});

var ReactComponent = React.createClass(ctx);

So when Reflux.mixinClass encounters componentWillMount or componentWillUnmount in the ctx, then it'll do something like this that keeps the implementation from the mixin:

// e.g. componentWillMount

// mixin as usual here

if (ctx.componentWillMount) {
  outputClass.componentWillMount = function() {
    ctx.componentWillMount();
    ListenerMixin.componentWillMount();
  }
}

Obviously have to figure out how to fix the binding context.

Thoughts?

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Jul 28, 2014

Member

Alternatively we could add a couple of template methods e.g.:

ListenerMixin.postComponentWillMount = function() {}; // stub
ListenerMixin.componentWillMount = function() {
    this.subscriptions = [];
    this.postComponentWillMount();
}

// Later 
Reflux.mixinClass({
    postComponentWillMount: function() {
        // override the stub
    }
});

But that'll make things a bit more confusing by adding functions that won't make sense to users who are accustomed to ReactJS.

Member

spoike commented Jul 28, 2014

Alternatively we could add a couple of template methods e.g.:

ListenerMixin.postComponentWillMount = function() {}; // stub
ListenerMixin.componentWillMount = function() {
    this.subscriptions = [];
    this.postComponentWillMount();
}

// Later 
Reflux.mixinClass({
    postComponentWillMount: function() {
        // override the stub
    }
});

But that'll make things a bit more confusing by adding functions that won't make sense to users who are accustomed to ReactJS.

@bripkens

This comment has been minimized.

Show comment
Hide comment
@bripkens

bripkens Jul 28, 2014

Contributor

The nice thing about mixins is that they are not overriding the life cycle methods.

A nice feature of mixins is that if a component is using multiple mixins and several mixins define the same lifecycle method (i.e. several mixins want to do some cleanup when the component is destroyed), all of the lifecycle methods are guaranteed to be called.
http://facebook.github.io/react/docs/reusable-components.html

This being said, the implementation that I pasted above should be sufficient. I'd willing to clean this up, write a few tests and send a PR if you are interested.

Contributor

bripkens commented Jul 28, 2014

The nice thing about mixins is that they are not overriding the life cycle methods.

A nice feature of mixins is that if a component is using multiple mixins and several mixins define the same lifecycle method (i.e. several mixins want to do some cleanup when the component is destroyed), all of the lifecycle methods are guaranteed to be called.
http://facebook.github.io/react/docs/reusable-components.html

This being said, the implementation that I pasted above should be sufficient. I'd willing to clean this up, write a few tests and send a PR if you are interested.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Jul 28, 2014

Member

Ah! I totally forgot that React had mixin capability in createClass.

Member

spoike commented Jul 28, 2014

Ah! I totally forgot that React had mixin capability in createClass.

@spoike

This comment has been minimized.

Show comment
Hide comment
@spoike

spoike Jul 28, 2014

Member

@bripkens Please do send a PR if you're up for it.

Member

spoike commented Jul 28, 2014

@bripkens Please do send a PR if you're up for it.

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