ES6ification #1976

mmcc · 2015-03-25T19:55:18Z

This isn't ready to merge yet...not even a little. I currently have a hacky way I'm going about getting tests working (custom es6 build), so that needs to be cleaned up and then another cleanup pass (or two...or 10).

I've purposefully kept this really light in terms of ES6 features. Once we've got this merged, I propose we take another pass at implementing a few more things that can be easy wins for readability, such as string interpolation and fat arrow functions. For this PR, I've only implemented import, let, and the occasional const. Because let and const are block scoped, I've moved almost all declarations to the point of assignment (where applicable).

There are some things such as VjsLib and VjsEvents that should be changed to just Lib and Events, and Utils should be merged with Lib.

So, if folks wouldn't mind taking a quick glance at 2,983 additions and 2,453 deletions across 66 files, that'd be wonderful. 👍

Related: #1720

heff · 2015-03-26T01:12:07Z

src/js/core.js

-    'textTrackSettings': {}
-  },
-
-  'language': document.getElementsByTagName('html')[0].getAttribute('lang') || navigator.languages && navigator.languages[0] || navigator.userLanguage || navigator.language || 'en',


Where did these language options go? They're not in options.js.

I uh...well...

lol. I added it back in my PR

heff · 2015-03-27T20:10:30Z

@videojs/core-committers we're looking to get this one in quickly so we can start migrating the other open PRs. I know it's big but try to get any comments you have in now.

coveralls · 2015-03-27T21:50:20Z

Changes Unknown when pulling 3f20078 on mmcc:es6ification into * on videojs:master*.

dmlap · 2015-03-30T13:55:42Z

sandbox/index.html.example


  <!-- LOAD VIDEO.JS SOURCE FILES IN ORDER -->
-  <script src="../build/source-loader.js"></script>
+  <script src="../build/temp/video.js"></script>


Will the sandbox require a build before being usable now?

Unfortunately yes. Browserify had no other option at least. But I guess now it might depend on what Chrome supports in ES6.

Right now Chrome supports a good deal of what we've already implemented, but I don't think any browsers support ES6 modules, so Browserify is still a hard requirement.

We should make sure we have a watchify npm script we can use.

We have grunt watch now. Does that work?

We could theoretically use grunt watch but we really do want watchify. watchify caches the build so rebuilds take minimal amounts of time. That way, you can watchify the build and then when you make a change and save you don't have to wait that long before you have to refresh the page.

Nice, that's great.

I use watchify and karma's watch to have continuous tests running in videojs-playlist

dmlap · 2015-03-30T15:49:55Z

This is a mega PR but I can't think of a better way to make this transition. I don't think we should shoot for a total, perfect conversion before pulling this in given the size of the migration. I say get it merged sooner rather than later so we can get more eyes on it.

gkatsev · 2015-03-30T15:51:24Z

Even if it gets merged in, we could always continue to comment here.

gkatsev · 2015-03-30T15:55:53Z

I would like a bit more time to a cursory glance at this PR, though.

mmcc · 2015-03-30T16:54:53Z

@dmlap yeah...I've had the same struggle. I hate both creating and reviewing PRs this big, but this is, unfortunately, an all or nothing transition.

heff · 2015-03-30T16:55:30Z

I mean, it's only 6000 changes. Just take your lunch hour and review 100 changes per minute.

mmcc · 2015-03-30T17:38:01Z

Also, just to reiterate, there are some other easy wins I think we can get from ES6, but I think they should be done in a different PR to minimize pain and suffering. This PR is just modules + let / const, the next PR can be default params and string interpolation. If we're going to move towards using classes and such, I think that should be its own PR.

coveralls · 2015-03-30T20:25:51Z

Changes Unknown when pulling 345445d on mmcc:es6ification into * on videojs:master*.

dmlap · 2015-04-02T03:46:15Z

@mmcc I'm with you on making this PR the minimal es6-ification and tacking the cool stuff in further updates. This is looking great!

mmcc · 2015-04-02T07:18:36Z

@dmlap sweet! @heff has already done some work towards the classes stuff, and we can get started on other sugar after this is merged in.

Speaking of which, it'd be great if we can get this merged in tomorrow. Everyone should get any final comments in as soon as you can so other 5.0 work can be done off this branch.

eXon · 2015-04-02T14:08:47Z

I'm seriously glad we are going in that direction. It's time to stop writing basic stuff like this and work on what really matters. I've been using ES6 with Babel in the past 3 months and it rocks. Big kudos @mmcc for doing that gigantic pull request.

I would love to see the for loops transformed into the for-of loop. Much much cleaner and easier to read. You can read more about it here.

An example for src/js/component.js

for (var i = 0; i < children.length; i++) {
    let child = children[i];

    let name, opts;
    if (typeof child == 'string') {
        // ['myComponent']
        name = child;
        opts = {};
    } else {
        // [{ name: 'myComponent', otherOption: true }]
        name = child.name;
        opts = child;
    }

    handleAdd(name, opts);
}

Would become:

for (let child of children) {
    let name, opts;
    if (typeof child == 'string') {
        // ['myComponent']
        name = child;
        opts = {};
    } else {
        // [{ name: 'myComponent', otherOption: true }]
        name = child.name;
        opts = child;
    }

    handleAdd(name, opts);
}

One other thing that would be interesting is also stop using our own event system. We should be using the EventEmitter inside the built-in event module. I'll create a separate issue to discuss this when this is merged.

The build looks fine to me:

Chrome 40.0.2214 (Windows 8.1): Executed 202 of 0 SUCCESS (2.188 secs / 2.124 secs)

=============================== Coverage summary ===============================

Statements   : 77.41% ( 2999/3874 )
Branches     : 63.2% ( 1130/1788 )
Functions    : 68.87% ( 489/710 )
Lines        : 77.14% ( 2923/3789 )
================================================================================


Done, without errors.

gkatsev · 2015-04-02T18:34:18Z

src/js/lib.js

-vjs.createEl = function(tagName, properties){
-  var el;
-
+var createEl = function(tagName, properties){
  tagName = tagName || 'div';


default arguments.

eXon · 2015-04-02T22:28:18Z

Bad news if we want to use for-of: Array.isArray is not even IE8 compatible. The pollyfill is one line of code though.

if (!Array.isArray) {
  Array.isArray = function(arg) {
    return Object.prototype.toString.call(arg) === '[object Array]';
  };
}

I don't think we need it at this point. I think it's worthless to talk about it at this point anyway.

gkatsev · 2015-04-02T22:33:54Z

@eXon we're thinking of making es5-shim as requirement, so, might not be an issue.

heff · 2015-04-04T00:01:26Z

There's some open discussions on this still but nothing that seems to be blocking and can't be pulled out into its own issue, so merging this in. (phew!)

@mmcc has updated the 5.0 change wiki page, and started the ES6 feature guide for contributors.

ES6 Classes (#1993) is the next big one. At this point no one appears to be against the idea, we just need a clean migration story, so I'm going to move forward with it and hopefully get it in quickly. If we want to be ready by the end of April we need to keep moving fast, so we could certainly use help knocking down more of the 5.0 issues.

ES6ification

gkatsev · 2015-04-04T03:39:29Z

Awesome!

carpasse · 2015-04-07T15:37:30Z

src/js/events.js

+    event.relatedTarget = event.fromElement === event.target ?
+      event.toElement :
+      event.fromElement;
+


This is a bit unrelated and I should raise an issue but ...
Can we add a check to see if event.relatedTarget exists??
Firefox supports it https://developer.mozilla.org/en-US/docs/Web/API/MouseEvent/relatedTarget and it does not support event.fromElement so in Firefox event.relatedTarget is undefined
:( We found this bug the other day.

I don't see why not. Want to make a PR for it?

Sure, where should I make the pull request? on this branch? is that possible?

This PR is merged now, so you can just make it against master.

Ups, I did not realize it was already merged. I will create the PR tomorrow morning.

@mmcc @carpasse Created an issue #2024 and starting the pull request.

heff reviewed Mar 26, 2015
View reviewed changes

heff mentioned this pull request Mar 29, 2015

Media tech for testing #1981

Open

dmlap reviewed Mar 30, 2015
View reviewed changes

mmcc mentioned this pull request Mar 30, 2015

WIP: Working browserify. Not backwards compatible. #1353

Closed

heff mentioned this pull request Mar 30, 2015

Added vjs.extend method #1909

Closed

mmcc force-pushed the es6ification branch from de2dbf5 to 85c505e Compare March 30, 2015 19:58

heff mentioned this pull request Mar 31, 2015

ES6 Classes #1993

Closed

mmcc force-pushed the es6ification branch from 345445d to 50fab66 Compare April 2, 2015 01:05

gkatsev reviewed Apr 2, 2015
View reviewed changes

mmcc force-pushed the es6ification branch from 50fab66 to 117db11 Compare April 3, 2015 22:15

cleanup and fixed options shadowing in player

f3036dd

mmcc force-pushed the es6ification branch from 117db11 to f3036dd Compare April 3, 2015 22:26

heff added enhancement confirmed labels Apr 3, 2015

heff added this to the v5.0.0 milestone Apr 3, 2015

heff added a commit that referenced this pull request Apr 4, 2015

Merge pull request #1976 from mmcc/es6ification

6e29bcc

ES6ification

heff merged commit 6e29bcc into videojs:master Apr 4, 2015

heff added a commit that referenced this pull request Apr 4, 2015

Added a line to the changelog for #1976

7f70f09

carpasse reviewed Apr 7, 2015
View reviewed changes

heff mentioned this pull request Apr 25, 2015

Add ES6 Transpiler? #1812

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES6ification #1976

ES6ification #1976

mmcc commented Mar 25, 2015

heff Mar 26, 2015

mmcc Mar 26, 2015

heff Mar 26, 2015

heff commented Mar 27, 2015

coveralls commented Mar 27, 2015

dmlap Mar 30, 2015

heff Mar 30, 2015

mmcc Mar 30, 2015

gkatsev Mar 30, 2015

heff Mar 30, 2015

gkatsev Mar 30, 2015

heff Mar 30, 2015 via email

gkatsev Mar 30, 2015

dmlap commented Mar 30, 2015

gkatsev commented Mar 30, 2015

gkatsev commented Mar 30, 2015

mmcc commented Mar 30, 2015

heff commented Mar 30, 2015

mmcc commented Mar 30, 2015

coveralls commented Mar 30, 2015

dmlap commented Apr 2, 2015

mmcc commented Apr 2, 2015

eXon commented Apr 2, 2015

gkatsev Apr 2, 2015

eXon commented Apr 2, 2015

gkatsev commented Apr 2, 2015

heff commented Apr 4, 2015

gkatsev commented Apr 4, 2015

carpasse Apr 7, 2015

heff Apr 7, 2015

carpasse Apr 7, 2015

mmcc Apr 7, 2015

carpasse Apr 7, 2015

mischizzle Apr 8, 2015

ES6ification #1976

ES6ification #1976

Conversation

mmcc commented Mar 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Mar 27, 2015

coveralls commented Mar 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff Mar 30, 2015 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlap commented Mar 30, 2015

gkatsev commented Mar 30, 2015

gkatsev commented Mar 30, 2015

mmcc commented Mar 30, 2015

heff commented Mar 30, 2015

mmcc commented Mar 30, 2015

coveralls commented Mar 30, 2015

dmlap commented Apr 2, 2015

mmcc commented Apr 2, 2015

eXon commented Apr 2, 2015

Choose a reason for hiding this comment

eXon commented Apr 2, 2015

gkatsev commented Apr 2, 2015

heff commented Apr 4, 2015

gkatsev commented Apr 4, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment