Rewrite to ES6 #38

nickygerritsen · 2016-01-17T14:32:30Z

Hi all!

Note: below text was the original text. Now that dash.js 2.0 is released, this PR is no WIP anymore.

This is a WIP to rewrite the whole plugin to use ES6 and browserify.

As a basis I started with the generator in a new directory and just moved code in it until it worked.

There are still some things that need to change and/or that are not working. Some of them I could really use some help with. Remarks:

I decided to use dash.js 2.x, which also uses ES6. They say it will be released this month so I don't see a big problem with this.
I forked the dash.js repo, so I could push a prerelease version of the ES6 code npm. Thus the plugin now depends on dash-es6-tmp, which we can change back again after dash.js 2.x is final.
Although Dash.js is ES6'ified, they still expose MediaPlayer on window and do not export it. It might make sense to create a PR for them to export it in all.js so we can import it and not use window. Agreed?
I couldn't find where they moved the DI context to, but it seems they default to {} now. So that's what I'm doing too.
I don't know what to do with the spec.js testfile, as I can't seem to find examples on how this is done nowadays.
As we are now shipping Dash.js in the javascript file, should we add something to the license somewhere?
Then some problems:
- UglifyJS hangs when --compress is given. No clue why but I have removed it and now it works.
- If you disable autoplay, an error is displayed and the stream will never play. The original plugin used to hide this error, as it is intended. I copied this, but somehow it is not working anymore. I can't seem to find out why, because it does work when autoplaying. If I manually hide the error and call player.play() the video does not play either. I tried calling play on the mediaplayer and then it does work. Maybe something they changed in the new Dash.js?
- The BBC video gives a black screen in Safari (OS X 10.11.4). However, it also does this on http://videojs.github.io/videojs-contrib-dash/ so I'm not sure if this is related. I tried another video and that one did work.
- On the example linked above Chrome switches to a higher resolution almost instantly if I go fullscreen. However on the index.html with npm run start it takes about a minute here. I'm not sure if this is Dash.js related or this plugin.
- The old source uses useVideoJSDebug and getWidevineProtectionData if they exist, but I can not find anyplace they are defined. So I added them back but no clue what they are for.
- The old source does a lot of console debug output, but I can't get that to work anymore. Maybe related to useVideoJSDebug?

Although there are quite some problems, I think this is a good beginning for this plugin to become ES6'ified :).

So @forbesjo / @dmlap / @imbcmdth any suggestions for these problems?

nickygerritsen · 2016-01-17T14:49:28Z

Ah I now see the Safari 9 issue has been repoted as #22

nickygerritsen · 2016-01-17T18:46:15Z

The autoplay error is (partly) a bug in Dash.js I believe. I created Dash-Industry-Forum/dash.js#1027 because I could reproduce the problem with only Dash.js.

nickygerritsen · 2016-01-17T18:58:43Z

Hmmm also the quality switching seems to be in Dash.js. I tried the reference player 2.0 (locally) and it also seems to load higher qualities way slower. This already seems to be broken in dash.js 1.6 and I've created an issue for that: Dash-Industry-Forum/dash.js#1028

misteroneill · 2016-01-17T20:02:49Z

I haven't had time to look through this PR yet, but it's great that you're working on it - thank you! We'll be sure to look it over and provide help/thoughts ASAP.

nickygerritsen · 2016-01-17T20:14:26Z

Glad to help :)!

forbesjo · 2016-01-21T20:10:21Z

Awesome thanks for doing this!

nickygerritsen · 2016-01-22T08:18:33Z

Btw #36 is already applied here too, as I accidentally worked of that code. But I guess that's not a big problem because we needed to do that manually otherwise anyway as it will never merge probably.

nickygerritsen · 2016-02-13T12:07:08Z

OK, dash.js 2.0 is released and I've integrated it into this PR.

Although they do not publish on NPM, we can use the tar.gz from the 2.0 release, which I did.

The following problems are still valid:

The old source uses useVideoJSDebug and getWidevineProtectionData if they exist, but I can not find anyplace they are defined. So I added them back but no clue what they are for.
The old source does a lot of console debug output, but I can't get that to work anymore. Maybe related to useVideoJSDebug?
I don't know what to do with the spec.js testfile, as I can't seem to find examples on how this is done nowadays.
As we are now shipping Dash.js in the javascript file, should we add something to the license somewhere?

UglifyJS does not hang anymore and all other points where dash.js related and they have fixed these.

So if we can answer the above things I think this PR is good :).

nickygerritsen · 2016-02-13T15:17:26Z

I fixed the merge conflict by rebasing, but it might have did something to 5226706, not sure

nickygerritsen · 2016-02-15T15:24:02Z

I fixed the linting errors, but had to comment out some stuff from karma.config.js

nickygerritsen · 2016-02-15T17:53:05Z

Not sure why the tests fail....
Locally they work

…version

nickygerritsen · 2016-02-16T09:42:36Z

My last pushes add the dash.js license and use a modified dash.js NPM package which does not use index.js as the main file. Using a relative path to dash.all.debug.js breaks when using this plugin as a ES6 module in another module.

I created Dash-Industry-Forum/dash.js#1173 to see whether they want to publish to NPM themselves and/or change the main file.

nickygerritsen · 2016-02-16T10:27:26Z

Another thing I'm having difficulties with now is that I do not know of any way of disabling dash.js debug output.

This can normally be done by calling a method on the mediaplayer after creating it but before initializing it it seems. But not sure how we should handle this then if we want to have this as an option.

forbesjo · 2016-02-16T21:06:51Z

Looks like you can:

get rid of integration.html and karma.config.js since the test files are browserified and detected.js is the main karma config now.
move the karma-browserify configuration into karma/detected.js if process.env.BROWSER_STACK_USERNAME exists (see https://github.com/videojs/video.js/blob/stable/test/karma.conf.js#L98)

nickygerritsen · 2016-02-16T21:11:52Z

Ah nice, will try this tomorrow :)

forbesjo · 2016-02-16T21:18:04Z

.travis.yml

+
+# Require firefox 43.0 for MSE
+addons:
+  firefox: "43.0"


Looks likes Travis has a latest option https://docs.travis-ci.com/user/firefox/

Yeah, is latest always what we want? I can change it then

nickygerritsen · 2016-02-17T07:34:39Z

OK I tried applying the suggestions. However I can not get it to work because somehow things go wrong with sinon...

I have no clue how to fix that.

gkatsev · 2016-02-17T07:38:56Z

You should pin sinon to 1.14.1 directly, that is, without the ^. That hopefully should fix all sinon-related issues.

* upstream/master: Set dash.js 1.6.0 as a dependency

nickygerritsen · 2016-02-17T08:59:05Z

OK now I really give up :(

nickygerritsen · 2016-02-19T09:16:57Z

btw dash.js is now published on NPM, so we can replace the tar.gz with a version.
However I saw @forbesjo was trying to fix Firefox, so I'm not sure whether I should push that to this PR?

forbesjo · 2016-02-19T14:33:56Z

You can keep pushing your changes to this PR while I try to figure out what the issue with Travis/Firefox is.

ksheurs · 2016-02-24T22:38:15Z

Will this also support Video.js 4.12.15, or only 5.x?

gkatsev · 2016-02-24T22:39:52Z

This will only support videojs 5.x.

forbesjo · 2016-02-26T00:52:14Z

@nickygerritsen I've opened a PR (#49) to separate the dash.js API updates from the ES6/browserification.

I think we should not bundle dash.js into the contrib-dash plugin but rather use the global dashjs.

nickygerritsen · 2016-02-26T07:20:18Z

@forbesjo this makes sense. When that is merged I will do some magic in this PR to fix related stuff :)

forbesjo · 2016-03-09T21:40:17Z

@nickygerritsen I've updated this plugin to use dash.js 2.0 and to use browserify in two other PRs, would you be able to rebase this PR?

nickygerritsen · 2016-03-09T21:50:37Z

Yes but probably next week or the week after

* upstream/master: Make the browserify shim a dependency Added Browserify support 2.1.0 Update contrib-dash for dash.js 2.0

nickygerritsen · 2016-03-15T13:02:17Z

OK pushed something that should at least merge everything. My rebase skills are not that good :(

nickygerritsen · 2016-03-15T13:09:22Z

Another option would be to just re-do my changes somewhat manually on top of the current master.

forbesjo · 2016-03-15T13:41:32Z

I'll grab your changes and do some rebase magic.

nickygerritsen · 2016-03-28T19:02:53Z

Any luck on the rebase?

forbesjo · 2016-03-28T19:55:23Z

Instead of rebasing (it's a very messy experience), I'll open an ES6 only PR and leave the generatorification for another day. Closing this PR.

nickygerritsen force-pushed the es6-browserify branch from 5a4871a to 88e4a20 Compare January 17, 2016 14:44

forbesjo added the needs: review label Jan 21, 2016

nickygerritsen changed the title ~~[WIP] Rewrite to ES6~~ Rewrite to ES6 Feb 13, 2016

nickygerritsen added 4 commits February 13, 2016 16:15

Beginning of rewrite to ES6 + browserify

0e9e592

Require firefox 43.0

86ac816

Use the latest dash.js release

2fd0607

Add compress back

57ee665

nickygerritsen force-pushed the es6-browserify branch from c14aed5 to 57ee665 Compare February 13, 2016 15:16

Fix linting issues

3db7f7b

Add dash.js license to banner and do not depend on specific video.js …

ac5a890

…version

Use NPM publish dashjs

dc6b848

forbesjo mentioned this pull request Feb 16, 2016

Set dash.js 1.6.0 as a dependency #43

Merged

forbesjo reviewed Feb 16, 2016
View reviewed changes

Nicky Gerritsen added 2 commits February 17, 2016 08:04

Use dashjs tag archive and path in package

d692a37

Use latest firefox and rewrite karma configuration

ad6d067

Nicky Gerritsen added 5 commits February 17, 2016 08:49

Fix the tests

90b5ea7

Pin sinon

f22f116

Merge remote-tracking branch 'upstream/master' into es6-browserify

769e07b

* upstream/master: Set dash.js 1.6.0 as a dependency

remove coverage check

53b9579

Add firefox plugin for Travis

b74418f

Use Dash NPM version

9f0c8e4

nickygerritsen mentioned this pull request Feb 20, 2016

Dash is not defined error with dash-2.0.0 #45

Closed

Merge remote-tracking branch 'upstream/master' into es6-browserify

a759893

* upstream/master: Make the browserify shim a dependency Added Browserify support 2.1.0 Update contrib-dash for dash.js 2.0

chemoish mentioned this pull request Mar 23, 2016

Dash.js integration #58

Closed

forbesjo closed this Mar 28, 2016

This was referenced Mar 28, 2016

Rewrite to ES6 #59

Closed

ES6 rewrite #60

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite to ES6 #38

Rewrite to ES6 #38

nickygerritsen commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

misteroneill commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

forbesjo commented Jan 21, 2016

nickygerritsen commented Jan 22, 2016

nickygerritsen commented Feb 13, 2016

nickygerritsen commented Feb 13, 2016

nickygerritsen commented Feb 15, 2016

nickygerritsen commented Feb 15, 2016

nickygerritsen commented Feb 16, 2016

nickygerritsen commented Feb 16, 2016

forbesjo commented Feb 16, 2016

nickygerritsen commented Feb 16, 2016

forbesjo Feb 16, 2016

nickygerritsen Feb 16, 2016

nickygerritsen commented Feb 17, 2016

gkatsev commented Feb 17, 2016

nickygerritsen commented Feb 17, 2016

nickygerritsen commented Feb 19, 2016

forbesjo commented Feb 19, 2016

ksheurs commented Feb 24, 2016

gkatsev commented Feb 24, 2016

forbesjo commented Feb 26, 2016

nickygerritsen commented Feb 26, 2016

forbesjo commented Mar 9, 2016

nickygerritsen commented Mar 9, 2016

nickygerritsen commented Mar 15, 2016

nickygerritsen commented Mar 15, 2016

forbesjo commented Mar 15, 2016

nickygerritsen commented Mar 28, 2016

forbesjo commented Mar 28, 2016

Rewrite to ES6 #38

Rewrite to ES6 #38

Conversation

nickygerritsen commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

misteroneill commented Jan 17, 2016

nickygerritsen commented Jan 17, 2016

forbesjo commented Jan 21, 2016

nickygerritsen commented Jan 22, 2016

nickygerritsen commented Feb 13, 2016

nickygerritsen commented Feb 13, 2016

nickygerritsen commented Feb 15, 2016

nickygerritsen commented Feb 15, 2016

nickygerritsen commented Feb 16, 2016

nickygerritsen commented Feb 16, 2016

forbesjo commented Feb 16, 2016

nickygerritsen commented Feb 16, 2016

forbesjo Feb 16, 2016

Choose a reason for hiding this comment

nickygerritsen Feb 16, 2016

Choose a reason for hiding this comment

nickygerritsen commented Feb 17, 2016

gkatsev commented Feb 17, 2016

nickygerritsen commented Feb 17, 2016

nickygerritsen commented Feb 19, 2016

forbesjo commented Feb 19, 2016

ksheurs commented Feb 24, 2016

gkatsev commented Feb 24, 2016

forbesjo commented Feb 26, 2016

nickygerritsen commented Feb 26, 2016

forbesjo commented Mar 9, 2016

nickygerritsen commented Mar 9, 2016

nickygerritsen commented Mar 15, 2016

nickygerritsen commented Mar 15, 2016

forbesjo commented Mar 15, 2016

nickygerritsen commented Mar 28, 2016

forbesjo commented Mar 28, 2016