IDs are globals. Use something safer to pick video.

[mikemaccana]

The first argument of videoJS is an ID, which becomes a global in almost all browsers (most developers still don't know this, which often means the breakage comes as a surprise). Users should be able to use something safer, ie, getElementsByClassName (and then item 0) or querySelector.

[heff]

Sorry, I'm not following. What do you mean by the id becomes a global, and what breakage happens?

[mikemaccana]

HTML IDs:

<p id="testwoo">woo</p>

become JS globals:

window.testwoo

It's terrible, but it's actually in the DOM spec, and it occurs on all browsers. What often happens is someone has an element with an id they want to enable some feature on, and that id conflicts with the name of that feature.

[gkatsev]

I think he means html id attributes. Which are supposed to be unique (though, browsers will handle multiples just fine).
Though, I agree that we should be able to provide a css selector and not necessarily just the element itself or the id of it.

[mikemaccana]

Yep, I do indeed mean HTML ID attributes. Whether they're unique of not isn't so much the issue, more that they're attached to window.

[mikemaccana]

More background: http://stackoverflow.com/questions/3434278/ie-chrome-are-dom-tree-elements-global-variables-here and http://www.whatwg.org/specs/web-apps/current-work/#named-access-on-the-window-object

[heff]

It looks like you can use any method that grabs the video element, and pass that in. No ID required.
http://jsbin.com/avEpuzAx/1/edit

And the data-setup init method works without IDs too.
http://jsbin.com/avEpuzAx/2/edit

So I guess we could drop the example ID in the docs when it's not needed. But something is still needed when the API is going to be used though.

videojs(document.getElementsByClassName('video-js')[0]); is pretty ugly for an API demo.
You couldn't used getElementsByTagName consistently because we kill the video tag when Flash is used.
I also don't love the idea of relying on methods that assume a certain order in the DOM. That feels brittle, especially with dynamic apps.

The example IDs we do use in the docs (which often just get copy/pasted) shouldn't be too much of a conflict threat with javascript var names, e.g. example_video_1. Or at least this is the first I've heard of this issue.

I'm leaning towards:

• dropping the ID in the doc examples when it's not needed
• suggesting the ID method when the API is needed
• warn about the potential conflict and provide alternative methods
  Though still open to other ideas.

@mikemaccana If you have any other info about the specific use case where you ran into this, that could be helpful.

[gkatsev]

Doesn't videojs create a generated id if one is not provided?

[gkatsev]

It looks like you can use any method that grabs the video element, and pass that in. No ID required.
http://jsbin.com/avEpuzAx/1/edit

Yeah, you can do that. What I think is better is to just have the user pass in the a selector to an element (or an element).

var video = videojs(document.querySelector('.my-vjs-video'));
var video2 = videojs('.my-vjs-video');
var video3 = videojs('#home_video');
var video4 = videojs(document.getElementsByClassName('my-vjs-video')[0]);

This would definitely be a breaking change, though, I would lean towards it being a change for the better.

And as I mentioned in the comment above, we should make sure not to add generated ids on the elements.

[mikemaccana]

I was thinking just passing the selector: videojs('.my-vjs-video') rather than the selection mechanism.

• If a selector string is passed, use the selector (and if it's a multiple selector, use the first item)
• If a NodeList or JQuery selection is passed, use the first item of that.

Anyone who uses 'videojs' itself as the ID would break, as I understand it.

[heff]

You can do 1, 3, and 4 today. I feel like 2 would be confusing with jQuery syntax. In videojs('.video-js').play(), it's not clear if that would play the first or all video-js players. I'd actually expect the latter, and I'd expect most jquery users would.

[mikemaccana]

Actually that's a good point: if it's unique, then enable videoJS for that item, if multiple items match, why not simply enable videoJS for all of them?

[heff]

@mikemaccana, because videojs() has always returned a single player instance, and our API docs have examples like:

var myPlayer = videojs('my_video_id');
myPlayer.play();

It might be easy enough to init each player when a classname was used in videojs(), but to have play() work across all players in the results, like a jQuery object, would be a big effort.

[gkatsev]

Not all jquery functions apply to the whole collection. A lot of them apply just to the first one. Yet another of jquery's inconsistencies.

[heff]

Yeah that's true. The example I can think of is val(), and that at least seems to make sense. videojs().play() would be more ambiguous.

[gkatsev]

the questions is what would videojs('.video-js') return?

• initialize all and return an array of players
• initialize all and return a special chainable collection of players
• initialize all and return just the first player
• initialize the first and return just the first player

Just some stuff I could come up with off the top of my head.
I think the first and last are the most reasonable solutions.

[heff]

Of those options I'd suggest option 4. But the main question for me is still whether or not it's worth adding support for classnames in the init function, when it's easy enough to use querySelector or jquery to pass in the element. There's code weight, testing, and support costs for every feature, so it needs to be worth it. This is the first request for this feature I've seen, so I'm not feeling the need too strongly yet. I'm still open to other arguments, but I'd suggest we put this on hold until we see more demand for it.

[mikemaccana]

@heff That makes sense. If videojs already supports elements, it may just be a documentation bug. From videojs.com:

"The first argument in the videojs function is the ID of your video tag. Replace it with your own."

If you accept elements, updating the docs could avoid the impression videojs needs an ID (and thus a global).

[heff]

Yeah, right now the docs essentially say that the ID is required. They should be updated to offer the element option as well. Changing the issue labels to docs+confirmed.

[mikemaccana]

👍

[mmcc]

I added some clarification to the setup docs based on this conversation (PR #999), so I'm going to go ahead and close this one. Thanks for the feedback, @mikemaccana!