Make initListeners more general. #2773

gkatsev · 2015-11-03T20:16:25Z

Also, look in the component's options themselves and not just children
for components to initialize.

Fixes #2767, has issues around #2772.

Also, look in the component's options themselves and not just children for components to initialize.

gkatsev · 2015-11-03T20:19:24Z

src/js/component.js

@@ -369,6 +369,10 @@ class Component {
      // If there's no .player_, this is a player
      let ComponentClass = Component.getComponent(componentClassName);

+      if (!ComponentClass) {


abort trying to make a component if it doesn't exist.

Should this throw an exception instead?

Elaborating a bit-- if a bad component was encountered midway through player creation, this would abort early and return a player missing a chunk of itself. It seems like a bad component name is a development-time-misconfiguration and it might be better to fail fast and force a fix.

Children could also, theoretically, be configured dynamically, in which case, we possibly don't want to throw.
However, now that I'm making sure that everything is a component below, I guess I probably don't even need to have this section here and leave it as what it was.

pam · 2015-11-03T20:20:09Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 13f524c
Build details: https://travis-ci.org/pam/video.js/builds/89085787

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-03T20:20:54Z

src/js/component.js

      }
+
+      workingChildren.concat(Object.getOwnPropertyNames(this.options_))


workingChildren is either the array of children or a list of the object properties from the children object.
We then want to concatenate it with the list of all names from options, since that could contain other children we want to add.

pam · 2015-11-03T20:22:02Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: a543d4d
Build details: https://travis-ci.org/pam/video.js/builds/89086398

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-03T20:22:04Z

src/js/component.js

      }
+
+      workingChildren.concat(Object.getOwnPropertyNames(this.options_))
+      .map((child) => {


get everything into the same format of [{name, options}, ...]

gkatsev · 2015-11-03T20:37:15Z

src/js/component.js

+        // See https://github.com/videojs/video.js/issues/2772
+        let c = Component.getComponent(child.opts.componentClass ||
+                                       toTitleCase(child.name));
+        return c && !Tech.isTech(c);


Ignore techs and not components. Techs need to be ignored specifically because of #2772.

pam · 2015-11-03T20:44:29Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 8000460
Build details: https://travis-ci.org/pam/video.js/builds/89089642

(Please note that this is a fully automated comment.)

pam · 2015-11-03T20:55:01Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 1c82bc9
Build details: https://travis-ci.org/pam/video.js/builds/89087416

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-03T21:15:31Z

It's failing in IE9 and 10 because those don't have Object.getPrototypeOf. Looking for an alternative.

pam · 2015-11-03T21:32:56Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 80c5172
Build details: https://travis-ci.org/pam/video.js/builds/89099542

(Please note that this is a fully automated comment.)

pam · 2015-11-03T21:47:54Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: b4bc4b8
Build details: https://travis-ci.org/pam/video.js/builds/89101186

(Please note that this is a fully automated comment.)

misteroneill · 2015-11-03T21:50:29Z

src/js/component.js

-        Object.getOwnPropertyNames(children).forEach(function(name){
-          handleAdd(name, children[name]);
-        });
+        workingChildren = Object.getOwnPropertyNames(children);


Out of curiousity, is there a reason we're using Object.getOwnPropertyNames instead of Object.keys in many/most places (not just this PR)?

It's pretty much an incidental difference for our purposes: keys includes only enumerable properties where getOwnPropertyNames includes non-enumerable properties.

Seems like keys would be the right mechanism here to me.

Switched to use keys.

pam · 2015-11-03T23:52:12Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 59a509e
Build details: https://travis-ci.org/pam/video.js/builds/89125066

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-04T00:02:01Z

@pam retry

pam · 2015-11-04T00:07:01Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 59a509e
Build details: https://travis-ci.org/pam/video.js/builds/89127221

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-04T00:09:16Z

@pam retry

pam · 2015-11-04T00:13:26Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 59a509e
Build details: https://travis-ci.org/pam/video.js/builds/89128366

(Please note that this is a fully automated comment.)

misteroneill · 2015-11-04T18:22:33Z

No tests? 😄

gkatsev · 2015-11-04T18:34:06Z

@misteroneill what are tests? :P
Yeah, I guess I should add some.

pam · 2015-11-04T19:40:28Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: bfeb3a4
Build details: https://travis-ci.org/pam/video.js/builds/89292637

(Please note that this is a fully automated comment.)

misteroneill · 2015-11-04T19:43:06Z

Do we have sufficient coverage for the various permutations of children on components?

gkatsev · 2015-11-04T19:44:06Z

I'm updating tests for that currently. Currently there's an issue where you might end up with two components if add settings to a component (rather than disabling it).

We don't want to actually run uniq because if the childrens array has multiple items, we do want multiple items in that case. However, we don't want to add more children of a kind if it is also available in options. So, we filter those out.

pam · 2015-11-05T19:27:44Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 2056048
Build details: https://travis-ci.org/pam/video.js/builds/89505144

(Please note that this is a fully automated comment.)

misteroneill · 2015-11-05T21:19:53Z

LGTM!

dmlap · 2015-11-05T22:22:40Z

src/js/component.js

@@ -521,33 +528,65 @@ class Component {
        // Add a direct reference to the child by name on the parent instance.
        // If two of the same component are used, different names should be supplied
        // for each
-        this[name] = this.addChild(name, opts);
+        let newChild = this.addChild(name, opts);
+        if (newChild) {


If you agree with my suggestion above, this part could be reverted.

pam · 2015-11-06T18:56:23Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: 88f92b0
Build details: https://travis-ci.org/pam/video.js/builds/89707197

(Please note that this is a fully automated comment.)

pam · 2015-11-06T19:43:35Z

Tests passed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: CONFIRMED

Commit: a1583ee
Build details: https://travis-ci.org/pam/video.js/builds/89715046

(Please note that this is a fully automated comment.)

Test that it throws correctly.

pam · 2015-11-06T20:43:40Z

Tests failed. Automated cross-browser testing via Sauce Labs and Travis CI shows that the JavaScript changes in this pull request are: BUSTED

Commit: 6bf3bb4
Build details: https://travis-ci.org/pam/video.js/builds/89725739

(Please note that this is a fully automated comment.)

gkatsev · 2015-11-06T20:50:08Z

@pam retry

dmlap · 2015-11-06T21:28:16Z

Had a discussion about reducing the calculation of workingChildren a bit but otherwise LGTM

gkatsev added 2 commits November 3, 2015 15:14

Make initListeners more general.

13f524c

Also, look in the component's options themselves and not just children for components to initialize.

Only add a child if it actually succeeded.

a543d4d

gkatsev reviewed Nov 3, 2015
View reviewed changes

gkatsev added 2 commits November 3, 2015 15:24

Fix jshint.

1c82bc9

Add isTech to Tech and use that in initChildren

8000460

gkatsev reviewed Nov 3, 2015
View reviewed changes

Use __proto__ if no Object.getPrototypeOf

80c5172

Suppress proto in jshint in isTech

b4bc4b8

misteroneill reviewed Nov 3, 2015
View reviewed changes

Fixup Tech.isTech.

59a509e

gkatsev added 2 commits November 4, 2015 14:33

Tech.isTech(Tech) should return true.

be04ace

Add tests for Tech.isTech.

bfeb3a4

gkatsev added 3 commits November 4, 2015 14:59

Make sure not to include components multiple times

9138616

Add a test for new functionality.

2056048

gkatsev mentioned this pull request Nov 5, 2015

Tech registry #2782

Closed

dmlap reviewed Nov 5, 2015
View reviewed changes

Switch to use Object.keys

88f92b0

Use test components instead of controlbar.

a1583ee

gkatsev added 2 commits November 6, 2015 15:40

Throw is trying to add nonexistent child.

6bf3bb4

Test that it throws correctly.

add throws to jshint qunit globals

540087b

Simply condition for extra children.

1ff7e2e

gkatsev closed this in 5d754c9 Nov 6, 2015

gkatsev mentioned this pull request Nov 9, 2015

5.2.0 release proposal #2796

Closed

gkatsev mentioned this pull request Dec 2, 2015

Doc.s are Inaccurate with Respect to Configuration of Player Controls, Syntax Inconsistent #2876

Closed

gkatsev deleted the children-options branch December 2, 2015 21:17

		}

		workingChildren.concat(Object.getOwnPropertyNames(this.options_))

Make initListeners more general. #2773

Make initListeners more general. #2773

Conversation

gkatsev commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Nov 3, 2015

Choose a reason for hiding this comment

pam commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Nov 3, 2015

pam commented Nov 3, 2015

gkatsev commented Nov 3, 2015

pam commented Nov 3, 2015

pam commented Nov 3, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pam commented Nov 3, 2015

gkatsev commented Nov 4, 2015

pam commented Nov 4, 2015

gkatsev commented Nov 4, 2015

pam commented Nov 4, 2015

misteroneill commented Nov 4, 2015

gkatsev commented Nov 4, 2015

pam commented Nov 4, 2015

misteroneill commented Nov 4, 2015

gkatsev commented Nov 4, 2015

pam commented Nov 5, 2015

misteroneill commented Nov 5, 2015

Choose a reason for hiding this comment

pam commented Nov 6, 2015

pam commented Nov 6, 2015

pam commented Nov 6, 2015

gkatsev commented Nov 6, 2015

dmlap commented Nov 6, 2015