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

new build that outs source es6 files into es5 folder #3445

Closed
wants to merge 25 commits into
from

Conversation

Projects
None yet
3 participants
@gkatsev
Member

gkatsev commented Jul 20, 2016

Description

Use babel to compile the source into es5 so that packagers can use it properly.

Specific Changes proposed

Update to latest version of babel, switch to babel-register, add grunt-babel, simply karma browserify config. use babel-preset-es2015-loose.
Make main point at the new es5/video.js file and include the es5 build in the npm published folder. This probably should also be added to the bower build I guess?
There's still some issues around updating to babel 6 and using babel-preset-es2015-loose. Also, needs testing in IE8.
This will also address #2750 and related issues.

Requirements Checklist

  • Fix babel 6 issues
  • Test in IE8
  • Is es2015-loose preset required? Is es2015 preset ok?
  • eliminate sourcemaps (fixes #3106)
  • verify that build process works on windows
  • Run babel through the test folder also for a simpler build
  • Make the build closer to the plugin generator build
  • Reviewed by Core Contributors

The struck out ones can be done separately.

This was referenced Jul 20, 2016

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 1, 2016

Member

file sizes now:

$ ls -lah dist/
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   929K Aug  1 15:48 video.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   168K Aug  1 16:00 video.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   323K Aug  1 15:49 video.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    64K Aug  1 16:01 video.min.js.gz

$ ls -lah dist/alt
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   860K Aug  1 15:48 video.novtt.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   150K Aug  1 16:00 video.novtt.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   295K Aug  1 15:49 video.novtt.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    54K Aug  1 16:01 video.novtt.min.js.gz

and before:

$ ls -lah dist/
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   755K Aug  1 15:55 video.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   159K Aug  1 15:55 video.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   275K Aug  1 15:55 video.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    61K Aug  1 15:55 video.min.js.gz

$ ls -lah dist/alt
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   687K Aug  1 15:55 video.novtt.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   141K Aug  1 15:55 video.novtt.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   247K Aug  1 15:55 video.novtt.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    51K Aug  1 15:55 video.novtt.min.js.gz
old new delta old gz new gz gz delta
main 755 929 174 159 168 9
min 275 323 48 61 64 3
novtt 687 860 173 141 150 9
novtt min 247 295 48 51 54 3

The file sizes are different because babel 6 produces more correct es6 code. However, if you take a look at the gzipped sizes, they don't really change much. babel-transform-runtime can help reduce the file size from about 929K to about 830K in the main file but the output it produces isn't IE8 safe.

Member

gkatsev commented Aug 1, 2016

file sizes now:

$ ls -lah dist/
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   929K Aug  1 15:48 video.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   168K Aug  1 16:00 video.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   323K Aug  1 15:49 video.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    64K Aug  1 16:01 video.min.js.gz

$ ls -lah dist/alt
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   860K Aug  1 15:48 video.novtt.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   150K Aug  1 16:00 video.novtt.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   295K Aug  1 15:49 video.novtt.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    54K Aug  1 16:01 video.novtt.min.js.gz

and before:

$ ls -lah dist/
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   755K Aug  1 15:55 video.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   159K Aug  1 15:55 video.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   275K Aug  1 15:55 video.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    61K Aug  1 15:55 video.min.js.gz

$ ls -lah dist/alt
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   687K Aug  1 15:55 video.novtt.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   141K Aug  1 15:55 video.novtt.js.gz
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users   247K Aug  1 15:55 video.novtt.min.js
-rw-r--r--   1 gkatsevman  VIDMARK\Domain Users    51K Aug  1 15:55 video.novtt.min.js.gz
old new delta old gz new gz gz delta
main 755 929 174 159 168 9
min 275 323 48 61 64 3
novtt 687 860 173 141 150 9
novtt min 247 295 48 51 54 3

The file sizes are different because babel 6 produces more correct es6 code. However, if you take a look at the gzipped sizes, they don't really change much. babel-transform-runtime can help reduce the file size from about 929K to about 830K in the main file but the output it produces isn't IE8 safe.

Show outdated Hide outdated package.json
"babel": "^5.2.2",
"babelify": "^6.0.1",
"babel-cli": "^6.11.4",
"babel-plugin-inline-json-config-values": "^1.0.0",

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

There are two outstanding PRs against this repo right now. I'm going to wait a week or so before publishing my own version to use instead:
mwilliams-change/babel-plugin-inline-json-config-values#2
mwilliams-change/babel-plugin-inline-json-config-values#3

@gkatsev

gkatsev Aug 2, 2016

Member

There are two outstanding PRs against this repo right now. I'm going to wait a week or so before publishing my own version to use instead:
mwilliams-change/babel-plugin-inline-json-config-values#2
mwilliams-change/babel-plugin-inline-json-config-values#3

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

Also, the browserify-versionify stuff could be removed.

@gkatsev

gkatsev Aug 2, 2016

Member

Also, the browserify-versionify stuff could be removed.

@@ -47,7 +45,7 @@ module.exports = function(grunt) {
release: {
tag_name: 'v'+ version.full,
name: version.full,
body: require('chg').find(version.full).changesRaw
body: chg.find(version.full).changesRaw

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

this is so that the babel json config plugin doesn't try and replace this.

@gkatsev

gkatsev Aug 2, 2016

Member

this is so that the babel json config plugin doesn't try and replace this.

Show outdated Hide outdated test/unit/tracks/text-track.test.js
@@ -269,7 +269,7 @@ test('tracks are parsed if vttjs is loaded', function() {
xhr(options, fn) {
xhrHandler = fn;
}
});
}).default;

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

proxyquirify doesn't understand babel es6 modules.

@gkatsev

gkatsev Aug 2, 2016

Member

proxyquirify doesn't understand babel es6 modules.

loose: ['all']
})
]
transform: ['babelify']

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

gets configured via .babelrc.

@gkatsev

gkatsev Aug 2, 2016

Member

gets configured via .babelrc.

Show outdated Hide outdated src/js/tracks/track-enums.js
/* jshint ignore:start */
// we ignore jshint here because it does not see
// AudioTrackKind as defined here
export default { VideoTrackKind, AudioTrackKind, TextTrackKind, TextTrackMode };

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

this way isn't supported or recommended anymore.

@gkatsev

gkatsev Aug 2, 2016

Member

this way isn't supported or recommended anymore.

Show outdated Hide outdated build/grunt.js
babel: {
options: {
sourceMap: true,
presets: ['es2015'],

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Aug 2, 2016

Contributor

should be es2015-loose or have no value and use the .babelrc

@BrandonOCasey

BrandonOCasey Aug 2, 2016

Contributor

should be es2015-loose or have no value and use the .babelrc

This comment has been minimized.

@gkatsev

gkatsev Aug 16, 2016

Member

Yes, it should probably be blank. I think the issue was that other things in the build pipeline wanted sourceMaps from babel. I'll take a look at this.

@gkatsev

gkatsev Aug 16, 2016

Member

Yes, it should probably be blank. I think the issue was that other things in the build pipeline wanted sourceMaps from babel. I'll take a look at this.

@@ -129,7 +129,7 @@ setup.autoSetupTimeout(1, videojs);
*
* @type {String}
*/
videojs.VERSION = '__VERSION__';
videojs.VERSION = require('../../package.json').version;

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Aug 2, 2016

Contributor

I assume this is where the babel plugin comes in, my question is: Why even bother trying to make it a literal? We could just use it like this and have rollup shake off everything from the package.json that we don't use (effectively making it literal anyway).

@BrandonOCasey

BrandonOCasey Aug 2, 2016

Contributor

I assume this is where the babel plugin comes in, my question is: Why even bother trying to make it a literal? We could just use it like this and have rollup shake off everything from the package.json that we don't use (effectively making it literal anyway).

This comment has been minimized.

@gkatsev

gkatsev Aug 2, 2016

Member

It would mean that anyone who wants to require videojs will need to use rollup or whatever to exclude everything.
this is also the reason for removing browserify-versionify. It makes it bundler agnostic.

@gkatsev

gkatsev Aug 2, 2016

Member

It would mean that anyone who wants to require videojs will need to use rollup or whatever to exclude everything.
this is also the reason for removing browserify-versionify. It makes it bundler agnostic.

This comment has been minimized.

@BrandonOCasey

BrandonOCasey Aug 2, 2016

Contributor

ok that makes sense

@BrandonOCasey

BrandonOCasey Aug 2, 2016

Contributor

ok that makes sense

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 3, 2016

Member

Tests are broken because it's using the wrong version of the babel plugin.

Also, I just realized that I think this is time for a proper solution to loading in vttjs. But I think it should be a separate PR but must be released with it.

Member

gkatsev commented Aug 3, 2016

Tests are broken because it's using the wrong version of the babel plugin.

Also, I just realized that I think this is time for a proper solution to loading in vttjs. But I think it should be a separate PR but must be released with it.

@gkatsev gkatsev added this to the 3.12 build-improvements milestone Aug 3, 2016

@gkatsev gkatsev referenced this pull request Aug 3, 2016

Closed

Rework inclusion of vtt.js #3491

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 11, 2016

Member

Loose mode is nice because it actually saves us about 100K in the build output. Plus, better IE8 support with it.

Member

gkatsev commented Aug 11, 2016

Loose mode is nice because it actually saves us about 100K in the build output. Plus, better IE8 support with it.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 11, 2016

Member

And rebased against master.

Member

gkatsev commented Aug 11, 2016

And rebased against master.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 11, 2016

Member

And it passed in browserstack as well: https://travis-ci.org/videojs/video.js/builds/151562935

Member

gkatsev commented Aug 11, 2016

And it passed in browserstack as well: https://travis-ci.org/videojs/video.js/builds/151562935

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 16, 2016

Member

Looks like this needs to be rebased.

Member

gkatsev commented Aug 16, 2016

Looks like this needs to be rebased.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 17, 2016

Member

Looks like the current build system works on windows, with the caveat that the shell:lint requires videojs-standard to be installed globally, so, this should be changed.

Member

gkatsev commented Aug 17, 2016

Looks like the current build system works on windows, with the caveat that the shell:lint requires videojs-standard to be installed globally, so, this should be changed.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 17, 2016

Member

@misteroneill one thing I noticed when running this on windows through github desktop app. The ghook makes it impossible to actually push to your remote using the app. If we want to make it easier for users to contribute, this makes it harder.
Also, we probably want that step to run vjsstandard --errors to ignore the warnings.

Member

gkatsev commented Aug 17, 2016

@misteroneill one thing I noticed when running this on windows through github desktop app. The ghook makes it impossible to actually push to your remote using the app. If we want to make it easier for users to contribute, this makes it harder.
Also, we probably want that step to run vjsstandard --errors to ignore the warnings.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 17, 2016

Member

I believe this PR is done and ready to be reviewed/pulled in.

Member

gkatsev commented Aug 17, 2016

I believe this PR is done and ready to be reviewed/pulled in.

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 17, 2016

Member

Actually, maybe the github app thing is just my setup. Would be good if some else who has a windows machine could confirm.
Also, maybe the macOS app has similar issues.

Member

gkatsev commented Aug 17, 2016

Actually, maybe the github app thing is just my setup. Would be good if some else who has a windows machine could confirm.
Also, maybe the macOS app has similar issues.

@gkatsev gkatsev changed the title from WIP: new build that outs source es6 files into es5 folder to new build that outs source es6 files into es5 folder Aug 17, 2016

@misteroneill

This comment has been minimized.

Show comment
Hide comment
@misteroneill

misteroneill Aug 17, 2016

Member

@gkatsev Makes sense re: Windows and ghooks. Is it because warnings the process is not exiting properly in Windows?

EDIT: I have a Windows machine, I'll give it a shot this week/end if I have some downtime!

Member

misteroneill commented Aug 17, 2016

@gkatsev Makes sense re: Windows and ghooks. Is it because warnings the process is not exiting properly in Windows?

EDIT: I have a Windows machine, I'll give it a shot this week/end if I have some downtime!

@gkatsev

This comment has been minimized.

Show comment
Hide comment
@gkatsev

gkatsev Aug 17, 2016

Member

It works fine if I open the git shell and push. I really don't know why the github app wasn't working.

Member

gkatsev commented Aug 17, 2016

It works fine if I open the git shell and push. I really don't know why the github app wasn't working.

@misteroneill

This comment has been minimized.

Show comment
Hide comment
@misteroneill

misteroneill Aug 17, 2016

Member

LGTM 👍

Member

misteroneill commented Aug 17, 2016

LGTM 👍

@gkatsev gkatsev added confirmed and removed needs: LGTM labels Aug 17, 2016

@misteroneill

This comment has been minimized.

Show comment
Hide comment
Member

misteroneill commented Aug 17, 2016

LGTM on 6c65b26

@gkatsev gkatsev closed this in 9551853 Aug 17, 2016

@gkatsev gkatsev deleted the gkatsev:new-build branch Aug 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment