Combined components.js and component.js #4
Conversation
…ency. Changed it so components register themselves.
…old way, and added deprecation warnings for using components in the old way
|
I added |
| @@ -374,7 +372,7 @@ Component.prototype.addChild = function(child, options){ | |||
| // If there's no .player_, this is a player | |||
| // Closure Compiler throws an 'incomplete alias' warning if we use the vjs variable directly. | |||
| // Every class should be exported, so this should never be a problem here. | |||
| component = new components[componentClass](this.player_ || this, options); | |||
| component = new Component.components[componentClass](this.player_ || this, options); | |||
There was a problem hiding this comment.
Can we use getComponent here as well?
|
@heff not having the components available as |
|
But otherwise, this looks really good. |
…he components object directly
|
Awesome. On a side not, as I'm going through the code base, the new structure feels a lot better. |
| module.exports.registerComponent = Component.registerComponent; | ||
|
|
||
| // Expose but deprecate the window[componentName] method for accessing components | ||
| lib.obj.each(Component.components, function(name, component){ |
There was a problem hiding this comment.
This is the piece I was thinking could make it backwards compatible. See any issues with it?
There was a problem hiding this comment.
Ah, I see, so, all the current components would still be exposed on the videojs object?
We should also double check to make sure we aren't running into circular module inclusion.
|
I added the global module for accessing the window. There shouldn't be any more circular dependencies now that component and components live together, and the components aren't ever used internally from the main object. This seems like a safe way to make it backwards compatible to me. Once this looks good to you can you accept the pull request? |
|
Would you be able to also swap out the references to |
…ed a lot of jshint errors.
|
Done with the document change. This all probably needs a rebasing sooner than later. Also closure compiler probably won't ever play nice with this, so we should switch to uglify in this or before hand. Also, |
| @@ -12,6 +12,8 @@ | |||
| "undef" : true, | |||
| "laxbreak" : true, | |||
| "predef" : [ | |||
| "document", | |||
There was a problem hiding this comment.
do we need that here now that we're making specific variables for doc and window?
There was a problem hiding this comment.
Yeah...jshint was yelling at me for redefining read-only vars.
|
@heff I was confused for a second but then realized that you need to run it as |
|
Aha! Thank you. |
What do you think of this direction? I like it because it combines all the component logic into one file and takes the
requireout of the component.addChild function, which is what was creating the circular dependency.Also I changed it so components get registered in the file where they're created. I like that because it's more like
exports, instead of having to look in another file for which components can be reference by name (i.e. we could have components that are only used inside other components and don't get registered for use elsewhere).We could also combine the register and extend into one line, if that's preferable.
I guess we could also shorten it to
Component.register()if that's still clear enough.(I have not run tests on this)