Restore child components configured as true

[misteroneill]

Previously, child components could be configured using true, false, or an object. Support for true appears to have been lost in the transition to 5.0. This restores that capability.

[dmlap]

lgtm

[heff]

The major changes around this were still in 4.x, so I'm surprised this works in 4.12 and not 5.x. I think I felt this wasn't needed anymore after moving to children arrays (#1599 is the closest PR I can find). Still seems like since you can just use an empty object that it'd be nice to deprecate this instead, but I'm fine either way if you have a good reason to keep it.

[misteroneill]

I'm gonna close this as children should be an array going forward.

[misteroneill]

Alright, I'm reopening this now that I understand the whole situation better.

An empty object does work, but it doesn't really feel intuitive to me - especially when false is a possibility.

Additionally, I want to raise (re-raise?) the issue that having children support either an array or an object leads to all sorts of unclear situations. It means a developer needs to inspect the various component classes to see what their default configurations might be and construct objects or arrays as necessary to either work with or around the implicit deep merge of objects but not arrays.

[heff]

An empty object does work, but it doesn't really feel intuitive to me - especially when false is a possibility.

Works for me. I think true would really only get used if you were overriding a deeper false in the chain, or if someone just assumed that true was needed to include a child, which supports your intuitive comment.

Additionally, I want to raise (re-raise?) the issue that having children support either an array or an object leads to all sorts of unclear situations.

Originally children was always an object. It allowed you to pass down options to any depth of children if needed.

playerOptions = {
  children: {
    controlBar: {
      children: {
        volumeMenuButton: {
           vertical: true
        }
      }
    }
  }
}

Then #1070 and #1093 happened and we added the option to use an array, specifically to control ordering. We didn't immediately switch all children objects to arrays because that would have been a pretty big breaking change and also arrays didn't allow you to pass options to children.

Then #1599 happened and we can add options to children defined with an array. I believe it should work for deeper children too.

playerOptions = {
    controlBar: {
        volumeMenuButton: {
           vertical: true
        }
    }
}

This does not help if you have two children of the same type that need different options, but I've yet to see a case of that, and if you need that you can rebuild the array. children: [ { name: 'sameComponent', foo: true }, { name: 'sameComponent', foo: false }].

I don't think we should remove the object option just yet but if we want to mark it as deprecated I'm ok with that. I think the array version is the right way going forward. Anyone else want to weigh in?

[dmlap]

Deprecate the option-object but keep it around for 5.x sounds like the right path to me.

[misteroneill]

@heff Thanks a ton for the historical clarification here! I don't have any strong opinions here, but I do prefer the array approach because it allows better control of final structure/ordering.

I'll take it upon myself to document the component children changes for 5.0 and deprecation of the object option.

[heff]

Ok cool. I guess this one is good to pull in though, yeah?

[gkatsev]

Somewhat related, can we allow users to add in a reference to a component directly in the array and initialize it as needed? :)

[heff]

@gkatsev not sure I follow completely. Are you just saying allow for already-initialized components in the array? Or can you give an example?

[gkatsev]

Basically.

let AConstructorForVjsToInitialize = function() {
 this.color = blue;
};
let someButtonThing = new Button();
// ...
children: ['foo', someButtonThing, AConstructorForVjsToInitialize, 'baz']

This should also apply for the techOrder array :)

Regardless, it definitely should not hold up this PR.

[dmlap]

Seems like this one has gotten signed-off but I'll throw in my "LGTM" as well.

[heff]

@gkatsev re: children: ['foo', someButtonThing, AConstructorForVjsToInitialize, 'baz']

someButtonThing may already work. If not I don't see why it couldn't be added, though also options couldn't be passed to it from the parent if it's already initialized.

AConstructorForVjsToInitialize, if we're talking about non-Components, would be difficult since the child init process expects a handful of component functions to be there.