Vexflow: ES6 class syntax

[aaronmars]

After implementing ES6 modules, the next layer of this onion would be the class syntax.
This would, of course, be a much more invasive change, but, again, the logical structure of the library makes is so darn tempting...

Here's what I'm thinking:

• Enable babel in the Rollup workflow
• Change all of the Vex.Inherit(Class, SuperClass, ...) calls to export class Class extends SuperClass { ... }
• Change all static properties to static getters and setters
• Change function declarations to the new class syntax (using static where appropriate)

Again, this can be done with source changes only. The Quint tests and visual regression tests would serve as verification that nothing functionally has changed.

[Silverwolf90]

Perfect, I was about to open a similar issue!

I experimented with lebab (with whatever the default settings are) and it will certainly save time, but it's far from complete. It doesn't know how to interpret the Vex.Inherit pattern, but it will recognize static methods and convert everything outside of Vex.Inherit into class syntax.

[Silverwolf90]

Here is modifier.js converted to class syntax with lebab

Note that the <Modifier>.CATEGORY pattern breaks. We'll need to modify it to use the constructor:

<Modifier>.constructor.CATEGORY

or change the pattern completely -- wouldn't it be simpler to set it to the instance?

[Silverwolf90]

Also note the weird comment bug... this didn't happen in an other file I tried.

EDIT: A few bugs around comments actually. Maybe this isn't going to work as well as I initially thought.

[0xfe]

Aaron, all good ideas. I think a good place to start is with one file -- let's figure out what the standard pattern should look like, and then move forward.

Cyril, thanks for the example -- why is the class wrapped in the const module? Can we do away with it?

[Silverwolf90]

Yes that was simply the automated output of lebab. Manual edits will be required.

Unfortunately, looks like lebab has a bug which strips comments with this pattern of setting the prototype:

Class.prototype = {
  a: function() {},
  b: function() {}
};

instead of individually setting functions to the prototype individually like this:

Class.prototype.a = function() {};
Class.prototype.b = function() {}

I mentioned this bug in the related lebab github issue

Until this is fixed, lebab might do more harm then good on files which use this pattern.

[Silverwolf90]

Here would be my suggested module + class style:

import { OtherThing } from './otherthing';

// Put an empty line between each multi-line method
export class Thing extends OtherThing {
  // Put static methods at the top
  static otherMethod() {
    // ... do some stuff here
  }

  // constructor is first thing after static methods
  constructor(x, y) {
    this.x = x;
    this.y = y;
  }

  // single-line methods acceptable if short
  // You don't have to put empty lines between consecutive single-line methods
  setX(x) { this.x = x; return this; }
  setY(y) { this.y = y; return this; }
}

Thing.staticProperty = 1;

[0xfe]

Looks good. I'd like to have private (non-method) helper functions in the file too, which won't pollute the global scope. E.g, Would L() below work, given that Thing hasn't been defined yet?

import { OtherThing } from './otherthing';

L() { if (Thing.DEBUG) Vex.L("Vex.Flow.Thing", arguments); }

// Put an empty line between each multi-line method
export class Thing extends OtherThing {
  // Put static methods at the top
  static otherMethod() {
    // ... do some stuff here
  }

  // First instance method should be constructor
  constructor(x, y) {
    this.x = x;
    this.y = y;
  }

  // single-line methods acceptable if short
  // You don't have to put empty line between consecutive single-line methods
  setX(x) { this.x = x; return this; }
  setY(y) { this.y = y; return this; }
}

Thing.staticProperty = 1;

[Silverwolf90]

Yes, that should work.

[Silverwolf90]

Note that you can only use the method shorthand in objects/classes.

L() { if (Thing.DEBUG) Vex.L("Vex.Flow.Thing", arguments); }

is a syntax error and would need to change to

function L() { if (Thing.DEBUG) Vex.L("Vex.Flow.Thing", arguments); }

provided we're not adding in other elements of ES6 yet.

[Silverwolf90]

Also, do we want to keep the constructor + init pattern that's currently throughout the codebase? I've never quite understood its purpose and it's very unidiomatic JS. I think we could move the logic in every init method into the constructor and get rid of the init methods completely.

[aaronmars]

Oh boy. I went ahead and did it all by hand. I hope I didn't jump the gun discussion-wise.
With regard to the things discussed here, I'll address what I've found. The structure you guys came up with is almost exactly what I landed on. Let me make a couple of changes/additions and see what you think.

I'm more a fan of the static getter and optional setter over the explicit declaration. At the very least, it allows you to make these things read-only by only supplying a getter. The other thing I added was the private helper function outside of the class scope.

Let me test that logging function with the static DEBUG parameter, and make sure the pattern is legit.

import { SuperThing } from './superThing';
import { OtherThing } from './otherthing';

// Private helper functions
function privateHelper() { // do stuff }
function L() { if (Thing.DEBUG) Vex.L("Vex.Flow.Thing", arguments); }

// Private static variables.
let _privateProp = 1;

// Put an empty line between each multi-line method
export class Thing extends SuperThing {
  // Static class properties
  static get staticProperty() {
    return _privateProp;
  }
  //  .... Only if necessary!!
  static set staticProperty() {
    _privateProp = prop
  }

  // Put static methods at the top
  static otherMethod() {
    // ... do some stuff here... like this, for example:
    privateHelper();
    // OR
    privateHelper.call(this);
  }

  // First instance method should be constructor
  constructor(x, y) {
    this.x = x;
    this.y = y;
    this.otherInstance = new OtherThing();
  }

  // single-line methods acceptable if short
  // You don't have to put empty line between consecutive single-line methods
  setX(x) { this.x = x; return this; }
  setY(y) { this.y = y; return this; }
}

Here's my branch with the first pass at the conversion. The tests _were_ passing perfectly, but then I merged the BoundingBoxComputation addition, and grunt test is failing... I'll look at that right now.

https://github.com/aaronmars/vexflow/tree/es6_babel

[aaronmars]

@Silverwolf90 , I removed that pattern in my conversions. I agree with you.
If all of those init functions become constructors directly, it allows us to use super() in the child constructors!

[Silverwolf90]

Awesome work @aaronmars! I've not seen that static property pattern before, but I really like it.

[Silverwolf90]

@aaronmars have you run npm install, because grunt-contrib-qunit got upgraded

[aaronmars]

Just saw that in the other issue... Thanks!

[aaronmars]

@Silverwolf90 , I'm thinking about that CATEGORY issue you mentioned. I don't see a need to define these statically on the class. As you suggested, it would be just as functional if they were defined like:

getCategory() { return 'categoryname'; }

Another option would be to bite the bullet, implement them as instance getters, and then refactor all of the call sites. I like that better, but it's a little more invasive (which is a funny statement looking at the diff as-is)

[0xfe]

Thanks Aaron. Let's leave CATEGORY static and use a static getter to reach it. They're used by other static methods within modifiers and it would get messy turning them into instance variables.

Looks like there are two parts to this: first is getting the new syntax in, then getting Cyril's eslint stuff. Here's how I propose we do this:

PR1: Aaron sends in new build rules and syntax for four files: accidental, stavenote, formatter, modifiercontext. Let's try to get consensus for that subset and merge.
PR2: Cyril sends in eslint rules applied to the same files, and we'll try to work through that.

After we agree on everything, lets migrate the rest of the files.

Thoughts?

[Silverwolf90]

That sounds like a good way forward to me!

[aaronmars]

OK! Yes, I agree that is a good approach. A linter commit isn't something that would be very fun to merge with this branch of mine.
I'll get the PR in today!

[aaronmars]

Here it is! PR: https://github.com/0xfe/vexflow/pull/364

[aaronmars]

Oh no. I read it wrong. That PR has everything in it. Having said that, the pattern is very consistent throughout (with the exception of vex.js and tables.js).

If there are any changes to the template, I'll bite the bullet and make them everywhere. My grep skills are pretty good after these refactors!

[Silverwolf90]

Now that https://github.com/0xfe/vexflow/pull/364 has been merged I think we can close this issue. Thanks for all the fantastic work @aaronmars, this is a huge step for VexFlow!