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.

Sign up for GitHub

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

Require videojs-vtt.js via require rather than concat #3919

Merged
merged 2 commits into from
Jan 11, 2017
Merged

Require videojs-vtt.js via require rather than concat #3919

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82f59d1
cleanup build/grunt.js for browserify
brandonocasey Jan 5, 2017
e231325
remove build:dist and fix a typo
brandonocasey Jan 11, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 29 additions & 47 deletions build/grunt.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,6 @@ module.exports = function(grunt) {
],
tasks: ['copy:dist']
},
novtt: {
files: ['build/temp/video.js'],
tasks: ['concat:novtt']
},
minify: {
files: ['build/temp/video.js'],
tasks: ['uglify']
Expand All @@ -149,10 +145,6 @@ module.exports = function(grunt) {
files: ['src/css/**/*'],
tasks: ['skin']
},
babel: {
files: ['src/js/**/*.js'],
tasks: ['babel:es5']
}
},
connect: {
dev: {
Expand Down Expand Up @@ -326,32 +318,39 @@ module.exports = function(grunt) {
}
},
browserify: {
options: browserifyGruntOptions(),
build: {
Copy link
Member

Choose a reason for hiding this comment

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

so, build is now the main task and takes the job of both build and dist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, the difference between the two was that dist would replace the vtt.js string path with a url to the cdn. Since we will require vtt.js by default when we build it isn't really needed to do them separately

options: browserifyGruntOptions(),
files: {
'build/temp/video.js': ['es5/video.js']
}
},
dist: {
buildnovtt: {
options: browserifyGruntOptions({transform: [
['aliasify', {aliases: {'videojs-vtt.js': false}}]
]}),
files: {
'build/temp/alt/video.novtt.js': ['es5/video.js']
}
},
watch: {
options: browserifyGruntOptions({
transform: [
['browserify-versionify', {
placeholder: '../node_modules/videojs-vtt.js/dist/vtt.js',
version: 'https://cdn.rawgit.com/gkatsev/vtt.js/vjs-v0.12.1/dist/vtt.min.js'
}],
]
watch: true,
keepAlive: true,
}),
files: {
'build/temp/video.js': ['es5/video.js']
}
},
watch: {
options: {
watchnovtt: {
options: browserifyGruntOptions({
transform: [
['aliasify', {aliases: {'videojs-vtt.js': false}}]
],
watch: true,
keepAlive: true
},
keepAlive: true,
}),
files: {
'build/temp/video.js': ['es5/video.js']
'build/temp/alt/video.novtt.js': ['es5/video.js']
}
},
tests: {
Expand Down Expand Up @@ -390,14 +389,6 @@ module.exports = function(grunt) {
options: {
separator: '\n'
},
novtt: {
src: ['build/temp/video.js'],
dest: 'build/temp/alt/video.novtt.js'
},
vtt: {
src: ['build/temp/video.js', 'node_modules/videojs-vtt.js/dist/vtt.js'],
dest: 'build/temp/video.js'
},
ie8_addition: {
src: ['build/temp/video-js.css', 'src/css/ie8.css'],
dest: 'build/temp/video-js.css'
Expand All @@ -408,22 +399,23 @@ module.exports = function(grunt) {
logConcurrentOutput: true
},
tests: [
'watch:babel',
'shell:babel',
'browserify:tests'
],
dev: [
'shell:babel',
'browserify:watch',
'browserify:watchnovtt',
'browserify:tests',
'watch:novtt',
'watch:skin',
'watch:dist',
'shell:babel'
'watch:dist'
],
// Run multiple watch tasks in parallel
// Needed so watchify can cache intelligently
watchAll: [
'watch',
'browserify:watch',
'browserify:watchnovtt',
'browserify:tests',
'karma:watch'
],
Expand Down Expand Up @@ -508,14 +500,13 @@ module.exports = function(grunt) {
grunt.loadNpmTasks('chg');
grunt.loadNpmTasks('grunt-accessibility');

const buildDependents = [
grunt.registerTask('build', [
'shell:lint',
'clean:build',

'babel:es5',
'browserify:build',
'concat:novtt',
'concat:vtt',
'browserify:buildnovtt',
'usebanner:novtt',
'usebanner:vtt',
'uglify',
Expand All @@ -528,18 +519,11 @@ module.exports = function(grunt) {
'copy:swf',
'copy:ie8',
'vjslanguages'
];

grunt.registerTask('build', buildDependents);

grunt.registerTask(
'build:dist',
buildDependents.map(task => task === 'browserify:build' ? 'browserify:dist' : task)
);
]);

grunt.registerTask('dist', [
'clean:dist',
'build:dist',
'build',
'copy:dist',
'copy:examples',
'zip:dist'
Expand Down Expand Up @@ -575,9 +559,7 @@ module.exports = function(grunt) {

// Run while developing
grunt.registerTask('dev', ['connect:dev', 'concurrent:dev']);

grunt.registerTask('watchAll', ['build', 'connect:dev', 'concurrent:watchAll']);

grunt.registerTask('test-a11y', ['copy:a11y', 'accessibility']);

// Pick your testing, or run both in different terminals
Expand Down
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
"xhr": "2.2.2"
},
"devDependencies": {
"aliasify": "^2.1.0",
"babel-cli": "^6.11.4",
"babel-plugin-inline-json": "^1.1.1",
"babel-plugin-transform-es3-member-expression-literals": "^6.8.0",
Expand Down
19 changes: 17 additions & 2 deletions src/js/tech/tech.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { bufferedPercent } from '../utils/buffer.js';
import MediaError from '../media-error.js';
import window from 'global/window';
import document from 'global/document';
import {isPlain} from '../utils/obj';

/**
* An Object containing a structure like: `{src: 'url', type: 'mimetype'}` or string
Expand Down Expand Up @@ -522,13 +523,27 @@ class Tech extends Component {
*
* @fires Tech#vttjsloaded
* @fires Tech#vttjserror
* @fires Tech#texttrackchange
*/
addWebVttScript_() {
if (!window.WebVTT && this.el().parentNode !== null && this.el().parentNode !== undefined) {
const vtt = require('videojs-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.

so, basically, this requires vttjs and then below, we make sure that it becomes available globally unless the vttjs option was provided. In the novtt build, a transform tells browserify not to include this file in the output?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, we use the vtt.js option if we have it, if not we use the require if it is available (which it won't be for novtt builds), otherwise we use the cdn url.

The transform that you mention works like this:
aliasify when passed false for a package alias will turn the require call into an empty object {} before browserify includes in in the bundle.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, just wanted to confirm I'm understanding it correctly.


// load via require if available and vtt.js script location was not passed in
// as an option. novtt builds will turn the above require call into an empty object
// which will cause this if check to always fail.
if (!this.options_['vtt.js'] && isPlain(vtt) && Object.keys(vtt).length > 0) {
Object.keys(vtt).forEach(function(k) {
window[k] = vtt[k];
});
this.trigger('vttjsloaded');
return;
}

// load vtt.js via the script location option or the cdn of no location was
// passed in
const script = document.createElement('script');

script.src = this.options_['vtt.js'] || '../node_modules/videojs-vtt.js/dist/vtt.js';
script.src = this.options_['vtt.js'] || 'https://cdn.rawgit.com/gkatsev/vtt.js/vjs-v0.12.1/dist/vtt.min.js';
script.onload = () => {
/**
* Fired when vtt.js is loaded.
Expand Down