flightjs/flight-with-child-components

FlightJS mixin for nesting components

flight-with-child-components's Issues

Don't use Component.mixin if the mixin is already mixed in to the Component

This line: L54. Could perhaps be argued that this is a bug in Component.mixin.

For example, a component uses this mixin to attach a child:

/**
 * Component One
 */

var withChildComponents = require('with-child-components');
var Child = require('child');

module.exports = flight.component(componentOne, withChildComponents);

function componentOne() {
  this.attributes({
    childSelector: '.child'
  });

  this.after('initialize', function () {
    this.attachChild(Child, this.select('childSelector'));
  });
}

And is then attached by another component that uses this mixin:

/**
 * Component Two
 */

var withChildComponents = require('with-child-components');
var ComponentOne = require('component-one');

module.export = flight.component(componentTwo, withChildComponents);

function componentTwo() {
  this.attributes({
    componentOneSelector: '.component-one'
  });

  this.after('initialize', function () {
    this.attachChild(ComponentOne, this.select('componentOneSelector'));
  });
}

At this point, withChildComponents attempts to mix itself into ComponentOne for a second time. Although Flight has a guard against mixing in a mixin more than once (compose.js L30), by the time that compose.mixin call is made in Component.mixin you've created a new Component.

This is a problem because while attachTo has a guard against attaching a second instance if you end up calling it twice on a node, this.attachChild doesn't have a guard against calling Component.mixin on a Component that has already mixed in withChildComponents. This means you create a new Component and attach a new instance each time this.attachChild is called (unlike how attachTo behaves). It's not common to intentionally call attachTo (or this.attachChild) multiple times, but it's safe to do with attachTo and is what I ended up doing in a hacky POC demo that surfaced this bug.

To quick-fix the issue, I checked if withChildComponents was listed in Component.prototype.mixedIn in order to conditionally make use of Component.mixin.

var mixins = Component.prototype.mixedIn || [];
var isMixedIn = (mixins.indexOf(withChildComponents) > -1) ? true : false;
(isMixedIn ? Component : Component.mixin(withChildComponents)).attachTo(destination, attrs);

Not sure where the best place to do this is (perhaps Flight itself, I have a patch to deal with it at that end too).

cc @angus-c

README nits

maybe use a regular selector in the second example, just to drive home that the API is identical to attachTo. Also add a comment to explain we're passing a custom teardown event

//or pass a custom teardown event...
this.attachChild(AnotherChildComponent, '#anotherNode', {
  teardownOn: 'someEvent'
});

Mixin withTeardown to the attachChild component

We could do the work to make sure the child component has withTeardown mixed in using Component.mixin's late binding.

UMD bundle

Make the mixin registration a UMD bundle, so it can be used anywhere. I am specifically interested here because I use browserify to bundle everything.

missing teardownOn attribute in child components

When child components are not pre-mixedin with withBoundLifecycle during attachChild, the teardownOn attribute is missing when using the old this.defaultAttrs.

Thanks!

Name?

Not entirely sure this is a good name. withChildren or withAttachChild maybe?

Suggest using late binding

Docs should suggest using .mixin, not a component withTeardown.