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

Include vtt.js using require/browserify #3794

Closed
wants to merge 4 commits into from
Closed

Include vtt.js using require/browserify #3794

wants to merge 4 commits into from

Conversation

brandonocasey
Copy link
Contributor

Description

Right now we concat vtt.js right into the video.js bundle.

Specific Changes proposed

  • add code that wraps around the require for vtt.js so that it can be disabled
  • use envify to enable or disable a build with vtt.js
  • use unreachable-branch-transform to remove the require call
  • use browserify-shim to wrap vtt.js if it is being included

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

* `browserify-shim`
* `envify`
* `unreachable-branch-transform`
script.src = this.options_['vtt.js'] || '../node_modules/videojs-vtt.js/dist/vtt.js';
if (process.env.VTT === false) {
script.src = this.options_['vtt.js'] || 'https://cdn.rawgit.com/gkatsev/vtt.js/vjs-v0.12.1/dist/vtt.js';
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this will be changed to true === false or false === false depending on envify configuration. From there unreachable-branch-transform will remove one of these if statements.

Copy link
Member

Choose a reason for hiding this comment

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

I dig it. Proper pre-processing! 👍

@@ -24,7 +25,17 @@ module.exports = function(grunt) {
plugin: [
['bundle-collapser/plugin'],
['browserify-derequire']
]
],
Copy link
Member

Choose a reason for hiding this comment

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

Does videojs-standard not complain about this trailing comma?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmmm I think that it should? maybe we ignore the whole build directory.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it does. Oh well. I don't have a problem with hanging commas. 😆

script.src = this.options_['vtt.js'] || '../node_modules/videojs-vtt.js/dist/vtt.js';
if (process.env.VTT === false) {
script.src = this.options_['vtt.js'] || 'https://cdn.rawgit.com/gkatsev/vtt.js/vjs-v0.12.1/dist/vtt.js';
}
Copy link
Member

Choose a reason for hiding this comment

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

I dig it. Proper pre-processing! 👍

@gkatsev
Copy link
Member

gkatsev commented Dec 21, 2016

I just realized that unfortunately, this doesn't solve all our needs :(
We need to be able to properly support webpack bundling as well as browserify.

@gkatsev
Copy link
Member

gkatsev commented Dec 27, 2016

Looks like both envify and unreachable-branch-transform can be run separately from browserify. We could run those on tech.js after running babel before publishing and that should make it so webpack can require it manually. I guess it'll have to be run manually after the alt and dist files are generated so that they don't step on each others toes.

@gkatsev
Copy link
Member

gkatsev commented Dec 27, 2016

Another option is to use something like https://github.com/erikdesjardins/babel-plugin-transform-dead-code-elimination, seems like it supports env vars by default and you can configure babel to run or not run plugins depending on env as well.

@gkatsev
Copy link
Member

gkatsev commented Dec 27, 2016

Looks like that package is going to be deprecated in favor of babili. It's possible we can use just parts of babili to do what we want. For example transform-inline-environment-variables or transform-node-env-inline with minify-dead-code-elimination or minify-guarded-expressions

@gkatsev gkatsev removed the confirmed label Dec 30, 2016
@brandonocasey
Copy link
Contributor Author

I think that we currently have no way to include vtt.js with webpack like your describing (as we concat it onto the browser dist). We default to not including videojs-vtt.js in the es5 dist. Do we want to require videojs-vtt.js it in the es5 dist by default?

@brandonocasey
Copy link
Contributor Author

See #3919

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants