Restore some globals

[dmlap]

It seems like they just save and retrieve the component from a private variable. Any background on why this change was made? Besides making it a bit more verbose to reference things like the techs it also makes enumerating available components seem like something that is discouraged.

[gkatsev]

One of the main benefits is that it allows us to better encapsulate the internal modules and not have circular dependencies.
Just having users of videojs attach variables to the videojs object shouldn't be encouraged or supported.
It's possible that something like listComponents() might be useful.

[dmlap]

I think you dodged the question :) Why shouldn't it be encouraged or supported?

[heff]

When switching to modules this was our major circular dependency. Component children were added by name off the videojs object. In Component, this.addChild(videojs[childName]). And videojs required Component to export it as videojs.Component. That made structuring files as CommonJS modules very difficult.

The solution was to make it so Component didn't look for other components on videojs and instead moved component registration to itself. It really is much cleaner this way.

It doesn't have to be a function. We could have other components just add themselves to Component.components, and the export components, videojs.components -> Component.components, the same way we're currently exporting getComponents. The benefit I see of having it as a function is future flexibility. We can change the internal workings of component registration down the line without changing the API, we could throw warning messages during registration, throw deprecation messages when someone gets a deprecated component. None of that is required though. @gkatsev may have additional benefits in mind.

[heff]

I'm gonna close this since it's a question but we can continue to discuss and reopen if a change is decided on.

[gkatsev]

Also, knowing when a component registered with us could be beneficial. Don't really have anything else in mind right now but we could add other functionality around it at a later date without making it a breaking change. If Users just add it directly to the videojs object we don't know.

Also, this falls under the "don't modify objects you don't own" mantra we should be striving for. Users of videojs don't own the videojs object, so, they shouldn't be adding random properties to it themselves.

[dmlap]

I'm don't quite follow the circular dependency issue but I'll trust you on that one. I think registering components as videojs[childName] or videojs.Component.component[childName] (or whatever) is preferable to the function style. We don't have a concrete use-case for any of the benefits of the function style today and they can all be achieved with the object-key style if it turns out we need them. The big benefit in my mind with the object-key style is discoverability. Having a list of all registered Components easily accessible is a great tool for learning how video.js works and reminding yourself whether the HTML tech is called html or Html or Html5 or html5.

[gkatsev]

We shouldn't encourage users to modify objects they didn't create themselves.
We can easily add a listComponents or getComponents function that still allows for discoverability and makes doesn't encourage users to modify objects they don't own.

[dmlap]

I don't really understand that objection. What is the difference between a function whose only purpose is to modify state and just telling users how to directly modify state? getBlah(), listBlah(), setBlah() seem like the J2EE solution to this problem.

[gkatsev]

I have been kind of skirting the issue because I wasn't sure how to say what I thought. But hopefully I've collected my thoughts enough so the following is intelligible.

---

There are several reasons why having a few simple functions for getting and registering the components is beneficial over just having users attach it to our object.
One of the benefits is that it provides a consistent API that allows users to register their components/plugins/techs/whatever. This consistent API also hides the implementation details of it. This means that after users upgrade their components to use this API if we have to make other breaking changes to components, they have one less thing to worry about.
Another benefit is that we could add some safety checks, again this can be done in a backwards compatible way in the future, to the component registration process. Like checking to see that you aren't registering a component of the same name as a component that already exists. Or we could go even further with this and add more features like pre-registration and post-registration hooks that allow you to modify the components dynamically, say, for instrumentation.
Further, videojs is our API object so having users pollute our API space isn't a good thing. This means that we need to support to some extent an API that isn't ours. It means we aren't free with our API choices for the future. See Array#contains and how it's Array#includes now in the ES7 spec proposal. I don't want to perpetuate this practice.
Having these methods allows for greater decoupling of the components so they don't even necessarily need to be registered with. For example, we could make the techOrder array take objects instead of just arrays and have videojs just instance that object if needed instead of looking for it in videojs[techName] or getComponent(techName).
Finally, I'm ok with having the listComponents function just return our underlying data structure that holds components (or better yet, a shallow clone of it). This way, if someone really wants to muck around with the internals they're able to do modify the objects but there's still some safety since this wouldn't be the default method for doing so. This also means that if someone is doing something weird and we break them because they're modifying the return value from listComponents we have more leeway in not fixing that issue. And with listComponents or whatever it ends up being named we could always just the internals of how components work but this function can still continue returning the same shape object for users to use.

In summary, having these functions allows us to have better, safer code that we can modify with more freedom without having to worry about whether we're going to break anyone.

[dmlap]

Thanks for the thoughtful response, @gkatsev. I still don't think making this change at this point is a good thing. Here's my reasoning:

1. It breaks backwards compatibility.
2. We don't have any plans to take advantage of the benefits you describe (and there are other ways of achieving them).
3. It makes the player harder to explore at runtime. For instance, we lose this:

If it weren't for the backward compatibility issue, I probably would agree with you. video.js is used on a lot of sites nowadays though and I think we need to have a higher bar for removing popular APIs.

[gkatsev]

players can easily be put back in but deprecated. I'm OK with keeping it around for now.
The old way of setting and getting components is a lot harder to handle, particularly, for IE8 usage.

[dmlap]

I think global options should be restored as well. It's another API breakage and given it's returning a shared, mutable object anyways the indirection is confusing.

[dmlap]

Based on the discussion above, I'm reopening this to restore videojs.players and videojs.options.

[heff]

Ok by me

[gkatsev]

We should consider logging a deprecation warning for the usage.

[heff]

Using a proxy? Is that supported places now?

[gkatsev]

Probably using getters/setters and the el trick for IE8.

[heff]

Both objects have arbitrary keys, so it seems like that would get pretty complicated.

[gkatsev]

Well, for players, we're only ever setting it in one place, so, that shouldn't be too hard.
Options is slightly harder but not significantly. Basically, we can use some form of Object.observe polyfill/shim.

[heff]

Got it. Works for me. Doesn't have to work everywhere in my opinion, just somewhere a dev will likely see it. i.e. no need for a shim for IE8.

[gkatsev]

if no shim for ie8, even better. However, some users still use .players and .options programmatically, so, it might be good to still have the ie8 shim for it.

[heff]

I'm just saying if it's IE8 we leave players/options alone, and not worry about adding an observer to throw a deprecation warning. I doubt there's cases where they'd only see the deprecation warning in IE8.

[gkatsev]

Oh, deprecation warning. I guess we were talking past each other. Yeah, I'm ok with leaving it out from IE8 if it isn't super easy to do.

[misteroneill]

I like the idea of using Object.observe on players and options, but there is an issue.

Re-exposing Player.players as videojs.players and globalOptions as videojs.options for backward compatibility is no problem. But using ES7's Object.observe alone would trigger warnings for legitimate/internal uses as well as deprecated uses because they share a reference. There might be a solution using a Proxy, but the number of browsers that support both Object.observe and Proxy is zero!

[gkatsev]

Why would it trigger warning for legitimate uses? There are no "legitimate" uses for the deprecated properties.

[misteroneill]

Because, for example, constructing a new Player (or calling videojs()) mutates Player.players which is a shared reference with videojs.players. Observing videojs.players also observes Player.players.

One solution might be to expose videojs.players as a Proxy for browsers that support it and that object can log its own deprecation warnings. That's Firefox and IE Edge, basically.

[gkatsev]

Ah, I see. I'm saying videojs.players should be it's own separate object. That one is the easier case because it should be read-only. Users of videojs shouldn't modify it. As part of making a new player we could update videojs.players or something.
And videojs.options could just be a reference to the same object we have stored for the global options. Maybe. It's definitely the harder and unclear part.

[misteroneill]

I see what you're saying, but I think I'm going to see what I can do with the Proxy approach I mentioned for browsers that support it (otherwise exposing the private objects directly).

[gkatsev]

I think the proxy approach won't give us the browser support we would want.

[misteroneill]

In what way? The only limitation I see is that browsers that don't support Proxy wouldn't see deprecation warnings, but they would start to as they adopt the ES6 standard.

[gkatsev]

Maybe we're talking past each other. Would the Proxy be using just for the deprecation warning?

[misteroneill]

Yeah, that's the idea. Sorry if it wasn't clear! I'll have a commit for preliminary review momentarily.

[gkatsev]

No worries. Just using it for deprecation warning is fine by mean. I see what you mean about "legitimate" cases now.

[misteroneill]

Here's the sort of thing I'm thinking.

[heff]

I believe this has been fully addressed at this point with #2395. Let me know if you think otherwise.

• videojs.players and videojs.options have been restored
• videojs[ComponentName] = ... can't be restored without creating a circular dependency. We could still export each component on videojs, but I don't think they're retrieved like that very often.