marionettejs / backbone.marionette

The Backbone Framework
https://marionettejs.com
Other
7.07k stars 1.26k forks source link

Please add RadioMixin to export #3642

Closed gam6itko closed 3 years ago

gam6itko commented 5 years ago

This functionality required in my tasks very often. Can you add it to export along with View, Region, Object?

paulfalgout commented 5 years ago

Seems reasonable. Can you tell me more about your use case? The mixin utilizes bindEvents and bindRequests so we'd need to export those as well with a public mixin. But then the quest is should the mixin also have Backbone.Events mixed in or are you thinking this would only be used on Backbone related objects?

bazineta commented 5 years ago

Just a comment here that while bindEvents gets automagically cleaned up, bindRequests, so far as I can see, still requires manual cleanup, i.e., calling unbindRequests in onDestroy, for example. Thus, if I'm still correct here and this does get exported, we should document the need for cleanup.

gam6itko commented 5 years ago

@PaulCapron I going to use it with View and CollectionView objects. Backbone.Events mixin is no needed, in my opinion

paulfalgout commented 5 years ago

@bazineta cleanup for bindRequests where the radio mixin API is present happens here here: https://github.com/marionettejs/backbone.marionette/blob/master/src/mixins/radio.js#L39 If anything it cleans up too much, but realistically nothing should happen after destroy so it seems ok.

The mixin isn't by default on the views because requests cannot be duplicated, but views often are. That said, to manually create essentially the same API on any view is:

const MyRadioView = View.extend({
  radioRequests: {
    'foo': 'getFoo'
  },
  initialize() {
    const channel = Radio.channel(this.channelName);
    this.bindRequests(channel, this.radioRequests);
  },
  onDestroy() {
    Radio.stopReplying(this.channelName, null, null, this);
  }
});

If we were to create some sort of mixin we'd have to do something like:

const MyRadioView = View.extend({
  radioRequests: {
    'foo': 'getFoo'
  },
  initialize() {
    this.initRadio();  // this would be a requirement to init the mixin
  }
});

_.extend(MyRadioView.prototype, RadioMixin);
// or could be abstracted to something like `MyMixedRadioView = RadioMixin(MyRadioView)`
bazineta commented 5 years ago

@paulfalgout We often follow this pattern instead, where we want the channel to be private; it's then injected into the subviews. Basically, it's being used as a private communications channel between what may be distant views in an arbitrarily deep hierarchy.

const MyRadioView = View.extend({
  radioRequests: {
    'foo': 'getFoo'
  },
  initialize() {
    this.channel = new Radio.Channel();
    this.bindRequests(this.channel, this.radioRequests);
  },
  onDestroy() {
    this.unbindRequests(this.channel, this.radioRequests);
  }
});
paulfalgout commented 5 years ago

Yep that's a good pattern, and honestly one I'd probably recommend over needing an additional radio mixin. There is one issue with any of these kinds of things, but if radioRequests is a function (which the radio mixin supports) that creates a function instance, then the unbind may not work because the handler functions will be different instances.

paulfalgout commented 3 years ago

This is handled in v5 https://github.com/marionettejs/marionette