Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Working browserify. Not backwards compatible. #1353

Closed
wants to merge 5 commits into from

Conversation

gkatsev
Copy link
Member

@gkatsev gkatsev commented Jul 17, 2014

DO NOT MERGE.

This is the work-in-progress for browserifying videojs. The bundle.js file is built using

browserify -d --standalone 'videojs' . -o bundle.js

from within the videojs directory. Can also just run npm run build.

This will create a bundle.js file which is a standalone (UMD) build of videojs. You can then load the bundle file directly in the browser together with the generated CSS and you have a working videojs player.
You can also consume videojs directly from another app/module that uses browserify by just require('video.js') and then when you run browserify it'll figure things out.

Unfortunately, this isn't backwards compatible right now. The various components aren't really exposed directly right now and if you add more components it won't find them on the videojs object since it excepts them on the ./components.js module. Though, more modules could be added by requiring the components.js module (require('video.js/components)` and then adding the extra component to it.
Probably a better way is to create a function for registering components and techs.

I'm going to see whether I can make it backwards compatible. Also, this needs to be cleaned up because we don't need all the var vjs = {}; /* ... */ module.exports = vjs everywhere. That var is superfluous.

@gkatsev
Copy link
Member Author

gkatsev commented Jul 17, 2014

I'm going to close the PR so that it doesn't accidentally get merged in.

@gkatsev gkatsev closed this Jul 17, 2014
@heff
Copy link
Member

heff commented Jul 17, 2014

Nice man, that's a ton of work.

It seems like the challenge here is we're not just publishing a single module, we're publishing an ecosystem of modules that we want to be useable in the browser, post-compile.

What if every internal component had a dual export?

module.exports = myModule;
window.videojs.myModule = myModule;

It's kind of the same as having a main file that pulls in all modules.

window.videojs = {
  myModule1: require(myModule1),
  myModule2: require(myModule2),
}

The latter doesn't seem like a terrible way to do it either. Just less automatic.

@gkatsev
Copy link
Member Author

gkatsev commented Jul 18, 2014

The problem with that is that we explicitly do not want to export to window unless videojs is included via a regular script tag. If it's inlcuded via commonjs (browserify) or AMD, it shouldn't pollute the global namespace.

@gkatsev gkatsev reopened this Jul 18, 2014
@gkatsev
Copy link
Member Author

gkatsev commented Jul 18, 2014

I did some cleanup. Removed all the, now, unnecessary vjs = {} from each file.
Next I want to pull everything into its own file. I.e., the tracks.js will be split into 12 files

  • TextTrack
  • CaptionsTrack
  • SubtitlesTrack
  • ChaptersTrack
  • TextTrackDisplay
  • TextTrackMenuItem
  • OffTextTrackMenuItem
  • ChaptersTrackMenuItem
  • TextTrackButton
  • CaptionsButton
  • SubtitlesButton
  • ChaptersButton

Afterwards, I'll spend more time to see if I can make it backwards compatible (though, I don't think it can be done easily and well), so, we'd need to come up with a new way to register components and techs. I think probably the best way is to follow an approach similar to plugins, where we have a method on videojs that allows you register a component or tech. Probably best to have separate mechanisms for techs and for components.

And finally, I'd need to address the tests.

@heff
Copy link
Member

heff commented Jul 26, 2014

This is exciting and not small. Want to set up a hangout to talk through this?

Component.prototype.addChild = function(child, options){
var component, componentClass, componentName, componentId, components;

components = require('./components.js');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, what?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, this is a different file that includes all components. Not requiring itself. :-P

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, though, components.Component is itself.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, there's a circular dependency here between component.js and components.js. It seems we've just moved the issue up the chain. Would it help if every component registered itself like components.registerComponent('Component', Component) instead exporting them in components.js? And at the same time if components and component were the same file?

@heff
Copy link
Member

heff commented Jul 29, 2014

I'd like to come up with a cleaner naming convention then vjslib and vjsevents. Any ideas around that?

@gkatsev
Copy link
Member Author

gkatsev commented Jul 29, 2014

lib and events?

@heff
Copy link
Member

heff commented Jul 29, 2014

sure, that works for me. I'm sure things might change again if we bring in lodash.

@gkatsev
Copy link
Member Author

gkatsev commented Jul 29, 2014

I know some of the code that uses events has variables that are called events but it shouldn't be a huge change anyway. I originally went with vjslib and vjsevents to make sure it was unique.

And yeah, I'd love to break down lib.js into multiple files as well.

@heff
Copy link
Member

heff commented Jul 29, 2014

Probably best to have separate mechanisms for techs and for components.

Yeah, the tech mechanism could just be something that registers a component and then also adds it to the techOrder.

Maybe registerComponent and registerTech?

This doesn't currently seem to handle the custom component use case, e.g.

MyButton = Button.extend({});
videojs('my_id', { children: { 'myButton':{} } });

Which I know having a way to register components will help with, but that's a good API workflow to keep in mind as we're designing this system. And it doesn't have to look like that example if there's a better way, but it should be clean.

Two questions need to be answered from that example.

  1. How does one register a component so that a player can use it as a child?
  2. How does one access an class like Button to subclass it?

In the current implementation, without any magic, it looks like it would be:

videojs.components.MyButton = videojs.components.Button.extend();

@gkatsev
Copy link
Member Author

gkatsev commented Jul 30, 2014

MyButton = Button.extend({});
videojs('my_id', { children: { 'myButton':{} } });

What about maybe:

MyButton = Button.extend({});
videojs('my_id', { children: { 'myButton': MyButton } });

I.e., if the object is a function, assume it's the constructor function for creating that object.

But really, I'm not a fan of implicit things like this because in this case MyButton gets added globally and then its assumed that videojs will be able to access that global property.


  1. I guess a registerComponent method could take a name and a constructor.
videojs.registerComponent = function(name, component) {
  require('./components.js')[name] = component;
}

And then to init a component and add it as a child:

var foo = function Foo() {} // just some thing
var videojs = require('video.js');
videojs.registerComponent('myfoo', foo);

var player = videojs("home_video", {
  children: {
    myfoo: {}
  }
});

  1. We could have people require it, if using browserify. Something along these lines:
var Button = require('video.js/button');

But otherwise, we'd need to export it either as it is now (videojs.components.Button) or on the videojs object (videojs.Button) itself.

@heff
Copy link
Member

heff commented Jul 30, 2014

But really, I'm not a fan of implicit things like this because in this case MyButton gets added globally and then its assumed that videojs will be able to access that global property.

Do you consider anything added to the videojs object 'global'?

One thing to keep in mind here is this is currently a UI library as much as it is a video player, the goal being to allow people to build custom UI elements for the player. Once it's compiled everything needs to be exposed somehow (globally?), as much as any function on lodash needs to be accessible from _.functionName. Does that makes sense and do you agree with that?

@gkatsev
Copy link
Member Author

gkatsev commented Jul 30, 2014

Adding to the videojs object isn't necessarily global, but currently, the way videojs works, if components are attached to videojs directly rather than via a function, would create cyclical dependencies that break things. We definitely should strive for the best/easiest interface while also not painting ourselves into a corner. Exposing properties through the videojs object is OK, but we should not accept properties to be passed in via the videojs object unless we were doing something like how Backbone allows you to provide your own XHR handling by overriding Backbone.$. It will just make it a lot easier for us to maintain videojs if there aren't (m)any implicit things we have to worry about.

In that specific example, MyButton variables gets bound to the global scope. Did you maybe mean to add it to videojs? A la:

videojs.MyButton = Button.extend({});
videojs('my_id', { children: { 'myButton':{} } });

@heff
Copy link
Member

heff commented Aug 4, 2014

Mentioned this in chat but recording it here too:

A big issue with making video.js modular like you'd like is Flash. We have to pass Flash functions that are in global scope for it to fire events and errors. https://github.com/videojs/video.js/blob/v4.6.4/src/js/media/flash.js#L39
Not sure how we're going to get around that.

YouTube works the same way. https://developers.google.com/youtube/js_api_reference
"In addition, any HTML page that contains the YouTube player must implement a JavaScript function named onYouTubePlayerReady. The API will call this function when the player is fully loaded and the API is ready to receive calls. See the Event Handlers section for more details."

I guess it could be possible to make the functions player specific. Dynamically creating their names based on the player or swf ID. But they still need to be in global scope for flash to be able to trigger them. Maybe that's good enough?

@gkatsev
Copy link
Member Author

gkatsev commented Aug 4, 2014

Yep, that's what I mentioned as well. While we have to leak some stuff globally because of flash, we should minimize that as much as possible and even, as you mentioned, have a flash specific method per player/swf object.

@heff heff added the confirmed label Aug 27, 2014
@heff
Copy link
Member

heff commented Nov 10, 2014

It looks like testing a browserify-based project in the browser requires a bundle step first. Is that right? If so that seems like it could make debugging a test error more of a pain.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 10, 2014

Yes, but you can use watchify to have it autobuild and what not.

@heff
Copy link
Member

heff commented Nov 10, 2014

Would it be possible to use source maps or something to connect line numbers to the original source?

@gkatsev
Copy link
Member Author

gkatsev commented Nov 10, 2014

Yes, definitely. If you run browserify/watchify with --debug, it'll have sourcemaps.

@heff
Copy link
Member

heff commented Nov 10, 2014

There’s nothing we could do for IE8 though, right?

@gkatsev
Copy link
Member Author

gkatsev commented Nov 10, 2014

es5-shim.

@heff
Copy link
Member

heff commented Nov 10, 2014

I can’t tell if that answers my question about sourcemaps in IE8.

On another note. What do you think about cis-umd as an option for requiring videojs but not bundling it?
https://github.com/nicolashery/cjs-umd

@gkatsev
Copy link
Member Author

gkatsev commented Nov 10, 2014

Oh, sorry. Yes, sourcemaps aren't available in IE8. So, for that you'd want to just not minify.

cjs-umd or whatever other thing is ok but it only maybe solves the outside api. It's missing the possibly more important piece of having the internals of videojs be modularized themselves.

@heff
Copy link
Member

heff commented Nov 12, 2014

Gradually wrapping my brain around the whole problem set here.

There's two end-developer workflows that we need to have stories for.

  1. Script Includes - Publish Video.js and plugins as standalone scripts that get included in order in the page. Plugin scripts would require videojs and all its tools and components to already exist in the page, globally.
  2. CommonJS Modules - Include videojs and plugin modules using require and combine them to create a custom player. Videojs would NOT be exported globally by default.

At this point everything is built around the script includes workflow, and we add UMD to provide a module interface for video.js. We're in the process of shifting over to where everything is built around the modules workflow, and we need stories around what that looks like for external plugins, techs, and shared components, while maintaining support for the script includes workflow.

There's a few stages to this that I can see.

  1. Use modules and browserify for internal dependency management and building the package (replace build/source-loader.js). This will also make it easier to include external dependencies like lodash. At the end of this stage the compiled video.js package could look relatively the same to the outside world if we're still relying on global vars (we'd essentially have a dual export to window and module).
  2. Solve for issue where global vars are currently required (API breaking)
  3. Solve for how videojs plugin authors could also support the module pattern. Right now for plugins we still rely on the globally available videojs.plugin function and don't have a story for how plugins could be built with UMD, and how plugins would get access to videojs in that case if it's not global.
    3.5. Solve for how plugin authors would share components with videojs
    • Is Component a seperate project all together that could be a dependency of both videojs and a plugin? Is browserify clever enough to share the same code in that case, and share the same object where components get registered?
    • Do plugins use components on the videojs object that is passed to a plugin some how? Is videojs still a peer dependency in that case?
      As a side note, it'd be nice if Components could be written the same way both internally to video.js and externally.

I think this PR is close to the end of 2. I don't have a clear view yet of what 3/3.5 will look like but I think understanding that could influence earlier steps. Can you see that far ahead?

@gkatsev
Copy link
Member Author

gkatsev commented Nov 13, 2014

I have some ideas for 3. I'll write them up soon when I have some time. Hopefully by the end of this week.

@heff
Copy link
Member

heff commented Nov 13, 2014

This UMD-jQueryPlugin example looks like it could be a good starting point.
https://github.com/umdjs/umd/blob/master/jqueryPluginCommonjs.js

@mmcc
Copy link
Member

mmcc commented Nov 18, 2014

One issue that @heff and I were just brainstorming is how to get around the disparity between what's in our code base and plugin authors / users will potentially see (particularly if they don't go the Browserify route).

For instance, a dev that's just doing everything on window wants to add a custom button, so he goes and checks out the button component in our source. He would see require('./button'); and exports all over the place, but none of this would really help him out too much. Our idea was to help bridge the gap between the two by having our own videojs.require() function that would basically do very little but make the code look similar. At the end of the day it just returns something from the components hash (or wherever).

This does buy us some additional niceties as well:

  • The ability to deprecate components in a friendly way
  • Getting devs closer to The Videojs Way™ (kidding, but does get them a little closer to playing in the Browserify world potentially).
  • We can allow people to require modules we use, such as Lodash, in their code.

I think this is most of what we talked about...@heff am I missing anything?

@heff
Copy link
Member

heff commented Nov 18, 2014

I think that covers it. A plugin could then look like...

videojs.plugin('myPlugin', function(){
  var Button = videos.require('button');
  var _ = videojs.require('lodash'); // assuming we add lodash as a dep

  var MySpecialButton = Button.extend();
  // blah blah blah...do something with MySpecialButton
});

I like it because it can create a 1 to 1 mapping between our internal dependencies and what's available to plugin authors, instead of creating a separate ways to export components compared to other types of dependencies.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 18, 2014

A dev shouldn't need to know look at our code to use it. That's what the API and docs are for.

I don't think that it should be called require but I do agree with the concept. We should be able to ask videojs for the button component or whatever other component we want.

var Button = videojs.getComponent('button');

Or something which would be mirrored by a registerComponent or similar method for adding your own components.

I don't think we should expose dependencies directly. Though, if a user were using browserify, they would be able to do require('video.js/node_modules/lodash'). And if we lay things nicely, we could even expose things as:

var Button = require('video.js/components/button');

The jquery-umd plugin example is exactly what it should look like. But again, a plugin author doesn't have to target everywhere video.js works, though, it would be nice. Updating the plugin generator with this would be great. Especially with the addition of a build tool that would just auto-wrap the plugin with the appropriate header/footer.


If everyone just used browserify, our lives would be much easier :)

@mmcc
Copy link
Member

mmcc commented Nov 18, 2014

While I agree in principal that reading in the code shouldn't be necessary, you can look through issues, plugins, etc and see that's simply not the case in practice. A lot of that is partially entirely because our docs are lacking in areas, but...docs are hard.

Though, if a user were using browserify

The point of exposing dependencies directly like this would be for folks not using Browserify.

Either I'm terrible at reading, or you added things after I started typing, because that last line pretty much wraps everything I just said in a nutshell.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 18, 2014

I didn't add anything. I agreed with the general idea but didn't agree on the specifics.

@heff
Copy link
Member

heff commented Nov 18, 2014

@gkatsev can you go into more detail on why you think exposing dependencies directly is a bad idea?

@gkatsev
Copy link
Member Author

gkatsev commented Nov 18, 2014

because then our users will depend on our dependencies and then we will not be able to switch dependencies and that is the last thing we want to happen.

@mmcc
Copy link
Member

mmcc commented Nov 18, 2014

I disagree, I think this actually helps us in that regard. With this work flow we can give reasonable deprecation warnings and things along those lines as opposed to people using them anyway and us just giving them a ¯_(ツ)_/¯ when we break things.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 18, 2014

I don't think users should be relying on our dependencies. If they want lodash, they should include lodash. We shouldn't need a deprecation warning if internally we switched from lodash to underscore various utilities.

@gkatsev
Copy link
Member Author

gkatsev commented Nov 18, 2014

We should expose some methods, like .each or something but we shouldn't be giving people our dependencies because we'll never be able to get rid of that dependency beyond a major (breaking) update where if our dependencies are encapsulated, we can swap out the dependencies as much as we want and as long as the functionality stays the same, it wouldn't matter.

@heff
Copy link
Member

heff commented Nov 19, 2014

I agree with @gkatsev that we shouldn't make it the default that any video.js external dependencies are available that way for plugin devs. We don't want to lock ourselves into including specific external libs, especially for minor functionality. I'm on the fence with lodash specifically, whether we use 'videojs.require' or not, since it's so universal.

Either way we have a few other internal classes/libraries that could also be made available this way that don't fall under 'components' specifically. Starting with CoreObject (which is essentially Class). There's also been a few cases now where I've wanted an EventListener class that's between CoreObject and Component. Beyond that there's the fullscreen API and a host of additional utility functions that could potentially be organized this way. The option for all of these is either to 'globalize' them onto videojs (or a nested property), or register and load them dynamically through something like getComponent or videojs.require.

I'm liking the registration method because it allows us to more cleanly deprecate things like Matt said. I'm not sure how far that should go, but even with utility functions it's kind of a nice idea that it would take the main videojs object away from being another utility library, e.g. videojs.each().

I'm personally liking the videojs.require API because it's open to other library types, it's familiar, and it makes plugin development look a little closer to core development. Are there other positives or negatives to that or getComponent specifically?

we could even expose things as var Button = require('video.js/components/button');

That would be cool, and we should definitely make it possible, but I don't think we'll be able to write plugins this way and have it still support the 'Script Includes' workflow. At least I can't think of a clean way where the plugin would still share the submodules with core at run time.

@heff
Copy link
Member

heff commented Dec 8, 2014

See gkatsev#4 for some additional work on this.

I think we could move the discussion around how to expose internal modules to plugin developers to its own issue. We did have some additional interesting thoughts on that, like allowing plugins to state which other plugins it relies on, including versions.

@heff
Copy link
Member

heff commented Feb 28, 2015

We should handle this first before any of the other 5.0 tasks. gkatsev#4 still needs to be merged, and then there will probably be a bunch of merge conflicts. Might even be easier to use this as a guide and start from scratch. @gkatsev do you think you'll have time to address this soon or should we open it to whoever can get to it?

@mmcc
Copy link
Member

mmcc commented Feb 28, 2015

I think we need to decide on #1812 before going any further.

@heff heff mentioned this pull request Mar 10, 2015
@mmcc
Copy link
Member

mmcc commented Mar 30, 2015

Closing in lieu of #1976, which builds on this + ES6.

@mmcc mmcc closed this Mar 30, 2015
@gkatsev gkatsev deleted the browserify branch August 12, 2015 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants