Issue 6741 - Use ES2015 classes in lib/filterClasses.js

[Manish Jethani, 2018-06-09 13:03:32]

Patch Set 1

What do you think?

The general pattern is:

  /**
   * Description of this class
   */
  class Foo
  {
    /**
     * Description of the constructor
     * @param bar Description of parameter bar
     */
    constructor(bar)
    {
      /**
       * Description of property bar
       * @type {?string}
       */
      this.bar = bar;
    }

    /**
     * Description of method serialize
     */
    serialize() {}
  }

  exports.Foo = Foo;

  /**
   * Description of instance property something
   * @type {number}
   */
  Foo.prototype.something = 1;

  /**
   * Description of class property anotherThing
   * @type {?boolean}
   */
  Foo.anotherThing = null;

... and so on.

https://codereview.adblockplus.org/29803559/diff/29803560/lib/filterClasses.js
File lib/filterClasses.js (right):

https://codereview.adblockplus.org/29803559/diff/29803560/lib/filterClasses.j...
lib/filterClasses.js:37: /**
For instance properties that are always assigned in the constructor, we can keep
the documentation in the constructor as well. The prototype does not have these
properties now.

https://codereview.adblockplus.org/29803559/diff/29803560/lib/filterClasses.j...
lib/filterClasses.js:258: get type()
This needs to be a getter now.

https://codereview.adblockplus.org/29803559/diff/29803560/lib/filterClasses.j...
lib/filterClasses.js:547: ActiveFilter.prototype.domainSource = null;
Where the instance property is not always assigned (i.e. the value sometimes
must come from the prototype), we add it to the prototype just like before and
include the JSDoc comment above.

[kzar, 2018-06-12 11:46:15]

I mean sure, the new class syntax looks OK to me I guess. I'm not too excited
about switching to it, but I'm not against it either - assuming browser
support's there and the rest of the team agrees.

[Manish Jethani, 2018-06-12 11:59:15]

On 2018/06/12 11:46:15, kzar wrote:
> I mean sure, the new class syntax looks OK to me I guess. I'm not too excited
> about switching to it, but I'm not against it either - assuming browser
> support's there and the rest of the team agrees.

Yeah, I'm not a big fan of ES2015 classes either, but the way we are doing it
currently clearly could use some improvement. Initially I thought we could just
use Object.assign [1], but this is actually cleaner and also better for
documentation (JSDoc now correctly identifies properties instead of incorrectly
assuming them to be "global"). Overall I think just declaring a "class" as an
actual class at the language level is just neater.

Anyway, so I thought everyone already agreed to this?

[1]: https://codereview.adblockplus.org/29746555/