ES6 Default args and interpolation #2015

mmcc · 2015-04-07T00:55:15Z

As well as a some fat arrows thrown in for good measure. Feel free to shoot anything in here you don't like down and we can revert.

gkatsev · 2015-04-07T01:02:34Z

[Something interesting you can do with destructuring](http://babeljs.io/repl/#?experimental=false&evaluate=true&loose=true&spec=false&playground=true&code=var getFileExtension %3D %28src%29 %3D> 'video%2Fmp4'%0Afunction src%28{ src %3D arguments[0]%2C type %3D getFileExtension%28arguments[0]%29 }%29 {%0A console.log%28src%2C type%29%3B%0A}%0A%0Asrc%28'foo'%29%3B%0A%0Asrc%28{%0A src%3A 'foo'%2C%0A type%3A 'bar'%0A}%29):

var getFileExtension = (src) => 'video/mp4' // or the real implementation

function src({ src = arguments[0], type = getFileExtension(arguments[0]) }) {
  console.log(src, type);
}

src('foo');
// => foo video/mp4

src({
  src: 'foo',
  type: 'bar'
})
// => foo bar

mmcc · 2015-04-07T17:37:51Z

@gkatsev so I really like this, but fwiw I don't think this cleans up too much in the actual code base since this is spread across the techs. So far I've done this, but that seems to be about as generalized as we can get.

gkatsev · 2015-04-07T17:42:10Z

src/js/tracks/text-track-settings.js

@@ -165,119 +165,123 @@ function setSelectedOption(target, value) {
  target.selectedIndex = i;
 }

+// I know what you're thinking: "This should be a tagged template!". I thought the same thing
+// when I first took a look. However,


It's for suspense.

Kidding, I think I decided to confirm that I was right and never went back and finished the comment.

gkatsev · 2015-04-07T17:42:25Z

Yeah, it's a fun experiment. Not sure how terribly applicable it is to our uses.

heff · 2015-04-07T23:13:01Z

src/js/component.js

-    if (!options || options === true) {
-      options = {};
-    }
+    // Apparently sometimes options is just true, so we need that to make sure we still convert options to an object.


"Apparently" :-/

Yeah, you can do:

{ children: { controlBar: true, posterImage: false } }

The false bit is useful. I think the true might have just been for symmetry. Now that you should be defining children with arrays I think we can ditch the true option (but not false).

mmcc · 2015-04-10T22:03:12Z

@heff I think this one's good to go unless you want to take another pass before one of us disappears into the rebase pit

heff · 2015-04-11T21:01:33Z

src/js/component.js

      if (touchDistance > tapMovementThreshold) {
        couldBeTap = false;
      }
    }
  });

-  noTap = function(){
+  let noTap = ()=> {


I don't know how I missed these, but they're the same as the other example we reverted. I looked at how babel translates arrow functions and it at least doesn't do an extra fn.bind(), so that's good. I want to make sure I'm not just being a luddite here so if there's fault in my reasoning let me know. I think it's an important issue.

I still don't think the shortened syntax alone is benefit enough to warrant upping the javascript reading level for contributors. For most contributors JavaScript is probably still not their primary programming lang, and we'd be replacing one of the most most well known keywords with one of the least known that does the exact same thing (in this specific case).

Also, Babel makes named functions when translating arrow functions.

var noTap = function noTap() {}

That goes against the "Arrow functions are always anonymous" rule and could potentially create issues in IE8. Though I'm not sure how likely that is and I wish we could use named functions more for stack traces.

Also if you have a text editor with a function shortcut, there's not much benefit left over for this specific case. f then tab quickly in Atom makes an empty function.

I do think there are some cool examples of using arrow functions, like:

let square = x => x * x;

I would have a harder time arguing against that, and it even seems more readable than the example above for some reason.

And using it for it's this binding.

var boundFn = ()=> { this.foo(); }

But in all the examples here we seem to just be using it for it's shorter anonymous function syntax, which if that's correct then for consistency there's over 500 other anonymous functions in the codebase we would need to update too. Am I missing another benefit in these examples?

mmcc · 2015-04-21T21:41:25Z

Trying to rebase from master to pull in #1993 was just uh...not gonna happen. Basically blew everything away and started from scratch, walking through the old diff to make sure I at least got all of the original changes.

We're in the process of converting Video.js to ES6, and one of our devs assumed that this meant all string concatenation should be replaced with string templates. In some cases though this makes the code less readable. It's more obvious when you're using more complicated variables. ``` const rateName = `${this.player.getPlaybackRate()}x` // vs const rateName = this.player.getPlaybackRate() + 'x' ``` The general rule we landed on was: - When variables are being added to a string, use a string template - When a string is being added to a variable, use concatenation There could be a better way to say that. For the original example, see videojs/video.js#2015 (comment)

heff · 2015-04-22T01:13:18Z

src/js/control-bar/mute-toggle.js

@@ -64,9 +64,9 @@ class MuteToggle extends Button {

    /* TODO improve muted icon classes */
    for (var i = 0; i < 4; i++) {
-      Lib.removeClass(this.el_, 'vjs-vol-'+i);
+      Lib.removeClass(this.el_, `vjs-vol-${+i}`);


I prefer to be positive about things.

heff · 2015-04-22T01:23:04Z

Aside from the +i note this looks good to me! 👍

mmcc · 2015-04-22T20:45:42Z

+i is fixed. Interesting that it actually outputs something like this:

var bar = "foo bat " + +i;

Which is functionally the same. TIL.

mmcc · 2015-04-22T20:48:14Z

woops, didn't mean to close this.

mmcc force-pushed the es6-defaults-interpolation branch from 7d9b856 to bd2fb3f Compare April 7, 2015 17:15

gkatsev reviewed Apr 7, 2015
View reviewed changes

heff mentioned this pull request Apr 7, 2015

ES6 Classes #1993

Closed

heff reviewed Apr 7, 2015
View reviewed changes

heff reviewed Apr 11, 2015
View reviewed changes

mmcc added 2 commits April 21, 2015 10:33

Updating documentation comments (removing vjs references)

a34e1a9

string interpolation + default args

562ae81

mmcc force-pushed the es6-defaults-interpolation branch from 8e39112 to 562ae81 Compare April 21, 2015 19:52

heff mentioned this pull request Apr 22, 2015

Add note on concatenating a string to an existing value airbnb/javascript#320

Closed

heff reviewed Apr 22, 2015
View reviewed changes

moved time controls into their own directory

1d94676

mmcc force-pushed the es6-defaults-interpolation branch from 88f9fa0 to 1d94676 Compare April 22, 2015 20:42

mmcc closed this Apr 22, 2015

mmcc reopened this Apr 22, 2015

switching to use iojs for travis

d94e17e

mmcc closed this in e1e6994 Apr 22, 2015

heff mentioned this pull request Apr 23, 2015

ES6: Default options and string interpolation #2009

Closed

mmcc mentioned this pull request Apr 23, 2015

consider adding templating #357

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ES6 Default args and interpolation #2015

ES6 Default args and interpolation #2015

mmcc commented Apr 7, 2015

gkatsev commented Apr 7, 2015

mmcc commented Apr 7, 2015

gkatsev Apr 7, 2015

mmcc Apr 7, 2015

mmcc Apr 7, 2015

gkatsev commented Apr 7, 2015

heff Apr 7, 2015

mmcc commented Apr 10, 2015

heff Apr 11, 2015

mmcc commented Apr 21, 2015

heff Apr 22, 2015

mmcc Apr 22, 2015

heff commented Apr 22, 2015

mmcc commented Apr 22, 2015

mmcc commented Apr 22, 2015

ES6 Default args and interpolation #2015

ES6 Default args and interpolation #2015

Conversation

mmcc commented Apr 7, 2015

gkatsev commented Apr 7, 2015

mmcc commented Apr 7, 2015

gkatsev Apr 7, 2015

Choose a reason for hiding this comment

mmcc Apr 7, 2015

Choose a reason for hiding this comment

mmcc Apr 7, 2015

Choose a reason for hiding this comment

gkatsev commented Apr 7, 2015

heff Apr 7, 2015

Choose a reason for hiding this comment

mmcc commented Apr 10, 2015

heff Apr 11, 2015

Choose a reason for hiding this comment

mmcc commented Apr 21, 2015

heff Apr 22, 2015

Choose a reason for hiding this comment

mmcc Apr 22, 2015

Choose a reason for hiding this comment

heff commented Apr 22, 2015

mmcc commented Apr 22, 2015

mmcc commented Apr 22, 2015