ES6 Classes #1993

heff · 2015-03-31T21:47:56Z

~~This is built on #1976 so the commit to look at is b7f6934~~
An easier view of the changes here: mmcc#7

So far I just converted Component to an ES6 class, and then re-added init and extend so that all the subclasses would still work (all tests are passing). After fully implementing this we would't have either of those methods anymore.

Important notes

ES6 classes (through babelify) requires an Object.create shim. Before this we don't yet need to include the shim/sham combo, but after this we would.
This will remove the MyComponent.extend() static method, for all components, techs, etc., breaking that major API. We can't automatically add the static extend() method in the ES6 inheritance process like we're doing now for every sub class. We can manually add it for each component if we think that's important for backwards compatibility, maybe at least for techs. Otherwise we should introduce a videojs.extends/videojs.inherits for external projects that don't use ES6 classes. We can even just use Node's.

Please let me know any thoughts on this.

coveralls · 2015-03-31T21:52:44Z

Changes Unknown when pulling b7f6934 on heff:es6classes into * on videojs:master*.

heff · 2015-04-05T05:03:28Z

So apparently static methods are inherited in ES6 classes, so that makes the old extend() API really easy to add since it only needs to be added once.

Also I'm using the latest version of babelify but it's yelling at me for something I think it shouldn't.

createEl() {
  return super();
}

SyntaxError: slider/slider.js: Line 48: Direct super call is illegal in non-constructor, use super."createEl"() instead while parsing file

I instead have to do

createEl() {
  return super.createEl();
}

The first way works in the babel demo interface, so I'm not sure what's going on there.

gkatsev · 2015-04-05T05:29:01Z

I think the spec changed to be the second example. I can find out for sure later.

Had to modify some of the workflow because class syntax restrictions e.g. no `this` before super(). But all tests are passing.

heff · 2015-04-07T22:56:32Z

The ES6 class work has been updated and all classes have been converted. All tests are passing so it could be merged in. I broke out any individual class into its own file, and some of the library got reorganized because of that.

This was mostly a syntax and organization change, with almost no functionality change. However, there's a limitation where you can't use this before super in a constructor, and I had to change when some variables are set. I was pretty careful about the order of things so I don't believe anything functionality should have changed around that.

However, I've realized we should be using options more to pass info to super classes, instead of setting values on the prototype. For example we have a bunch of featuresX properties on techs, and we really should be passing that through options.

I also renamed MediaTechController to Tech and the /media/ directory to /tech/ for clarity. But I also aliased Tech to MediaTechController so old techs won't break on that alone.

I have not converted over all static methods, and not sure if we need to.

Also, this and #2015...

mmcc · 2015-04-07T23:43:13Z

mmcc · 2015-04-08T22:48:57Z

src/js/big-play-button.js


-export default BigPlayButton;
+Button.registerComponent('BigPlayButton', BigPlayButton);
+export default BigPlayButton;


Am I reading this right that this is removing the trailing newline? I won't comment on every instance of this, but I feel as strongly about a trailing newline as I do about removing empty white space.

Which is to say, very strongly. I'm pretty sure revolutions have been fought over the need for trailing newlines.

Ha, I guess that's what it is. And I guess Atom isn't configured to keep those for me. I'll update it.

Looks like it's enabled by default now, which would be weird if you're not seeing it.

mmcc · 2015-04-09T00:51:28Z

This isn't a problem with the PR itself, obviously, but this diff is useless.

mmcc · 2015-04-09T01:06:27Z

src/js/component.js

+    let parentOptions = parent.options();
+    let children = parentOptions['children'];
+
+    let handleAdd;


Why is this defined outside of the if block if it's only used within? The only other path is the else, which could easily be changed to Lib.obj.each(children, null); and honestly be more readable.

Wait a second, think the diff was screwing with me there, it doesn't look like there's any reason to define this outside of the if block.

It made sense when it was vars. The if block used children which was defined with parentOptions which was defined with parent. I didn't actually touch this part but I've cleaned it up for you. :)

heff · 2015-04-09T01:20:31Z

Yeah, the diff is bad. I could have avoided moving files for the first pass, but even in the files I didn't move, the complete restructuring of the class makes it impossible to tell what exactly changed.

mmcc · 2015-04-09T01:21:46Z

It's a lot better if you switch to the split diff, so that's what I'm using now.

mmcc · 2015-04-09T04:26:45Z

src/js/component.js

-  this.el_ = null;
-};
+    // Remove element from DOM
+    if (this.el_.parentNode) {


Should we use the #el() method instead of the "private" var when just reading this (throughout the module)?

The official Closure Library style is to use the private references everywhere internally so compiler can mangle them, and then export the methods for anything you want accessible externally. That's not always obvious for other contributors, and I also started to worry that people would start using the private properties externally after seeing internal code, but then on the other hand I think uglify has an option to mangle private props now if we wanted to keep that part of the file size savings.

Either way it's open for a change.

mmcc · 2015-04-09T06:02:15Z

src/js/tech/html5.js

+
+  addRemoteTextTrack(options) {
+    if (!this['featuresNativeTextTracks']) {
+      return Tech.prototype.addRemoteTextTrack.call(this, options);


mmcc · 2015-04-09T06:20:42Z

I'll take another pass tomorrow, but first pass showed a few things that can refactored to super calls.

Just a note not necessarily related to this PR: things seem a bit inconsistent about when we use things like el_ and player_ versus their respective getter methods.

gkatsev · 2015-04-09T19:28:43Z

src/js/component.js

@@ -3,7 +3,7 @@
 *
 */

-import CoreObject from './core-object.js';
+// import CoreObject from './core-object.js';


remove if not needed.

heff · 2015-04-11T19:28:52Z

I guess the punishment for having a crappy diff and forcing people to look at every line of code is you get to address every style problem from ever :) There's at least fewer than I'd expect.

heff · 2015-04-14T20:06:21Z

Updated with the super calls and cleanup changes mentioned. I've moved some not directly related discussions to their own issues. Otherwise the important bits here are done so going to merge this in so we can move forward.

…s#1993" This reverts commit a02ee27.

This was referenced Apr 1, 2015

ES6ification #1976

Merged

Breaking apart files #2010

Closed

Converted all classes to use ES6 classes

9bc7d3b

Had to modify some of the workflow because class syntax restrictions e.g. no `this` before super(). But all tests are passing.

heff force-pushed the es6classes branch from 7d18181 to 9bc7d3b Compare April 7, 2015 22:38

mmcc reviewed Apr 8, 2015
View reviewed changes

mmcc reviewed Apr 9, 2015
View reviewed changes

gkatsev reviewed Apr 9, 2015
View reviewed changes

Cleaned up super method calls and some other minor style issues

52ac7e5

This was referenced Apr 14, 2015

Move default options into the constructor #2033

Open

Decide on a final var style #2034

Closed

heff closed this in a02ee27 Apr 14, 2015

mmcc mentioned this pull request Apr 21, 2015

ES6 Default args and interpolation #2015

Closed

libins added a commit to libins/video.js that referenced this pull request May 4, 2015

Revert "@heff converted all classes to use ES6 classes. closes videoj…

1c9cc62

…s#1993" This reverts commit a02ee27.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES6 Classes #1993

ES6 Classes #1993

heff commented Mar 31, 2015

coveralls commented Mar 31, 2015

heff commented Apr 5, 2015

gkatsev commented Apr 5, 2015

heff commented Apr 7, 2015

mmcc commented Apr 7, 2015

mmcc Apr 8, 2015

mmcc Apr 8, 2015

heff Apr 9, 2015

mmcc Apr 9, 2015

mmcc commented Apr 9, 2015

mmcc Apr 9, 2015

mmcc Apr 9, 2015

heff Apr 11, 2015

heff commented Apr 9, 2015

mmcc commented Apr 9, 2015

mmcc Apr 9, 2015

heff Apr 11, 2015

mmcc Apr 9, 2015

mmcc commented Apr 9, 2015

gkatsev Apr 9, 2015

heff commented Apr 11, 2015

heff commented Apr 14, 2015

ES6 Classes #1993

ES6 Classes #1993

Conversation

heff commented Mar 31, 2015

Important notes

coveralls commented Mar 31, 2015

heff commented Apr 5, 2015

gkatsev commented Apr 5, 2015

heff commented Apr 7, 2015

mmcc commented Apr 7, 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

mmcc commented Apr 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

heff commented Apr 9, 2015

mmcc commented Apr 9, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmcc commented Apr 9, 2015

Choose a reason for hiding this comment

heff commented Apr 11, 2015

heff commented Apr 14, 2015