kefirjs / kefir

A Reactive Programming library for JavaScript
https://kefirjs.github.io/kefir/
MIT License
1.87k stars 97 forks source link

ES6 classes #196

Closed 84564221 closed 7 years ago

84564221 commented 8 years ago

Following #184, what's your opinion on converting the prototype chains into ES6 classes?

export default class Dispatcher {
  constructor() {
    this._items = [];
    this._inLoop = 0;
    this._removedItems = null;
  }

  add(type, fn) {
    this._items = concat(this._items, [{type, fn}]);
    return this._items.length;
  }

  remove(type, fn) {
    //...
  }

  //...
}
rpominov commented 8 years ago

Sorry missed this issue somehow. Honestly I don't have any strong opinion here.

mAAdhaTTah commented 7 years ago

Is this something you'd still be interested in? I'm looking at extending some of the built-in streams for various purposes, and while I'm assuming class MyObservable extends Kefir.Observable should work, having actual parent classes may be beneficial for this.

polytypic commented 7 years ago

Note that Babel generates a lot of code and (uses) helpers for ES6 classes. It should already be possible to write ES6 classes (compiled with Babel) that extend Kefir.Observable. I used to do that, but then converted my code not to use ES6 classes after realizing how much extra stuff Babel generates for those.

mAAdhaTTah commented 7 years ago

Ah, ok. Good to know. I haven't started to extend the built-ins yet, but I'm seeing a couple place where that will be beneficial, but if it'll bloat the library, may not be worth it.

rpominov commented 7 years ago

Yeah, I also worry about the overhead Babel transformations will bring. And for the end user it should make no difference whether Kefir.Observable was created using class or function. You should be able to extend it just fine.

The only upside is that code may become a little more clear and idiomatic, but I still not sure whether this worth the work that'll need to be done (including making sure we didn't break anything), and potential Babel overhead.

mAAdhaTTah commented 7 years ago

If this is going to have perf implications, we should pass on this and maybe revisit it for a future major version, if desired.

Macil commented 7 years ago

I'd expect any overhead would be load-time only and insignificant. The class syntax really is just syntax sugar for the same prototype-manipulation that pre-ES6 code does.

mAAdhaTTah commented 7 years ago

There are a couple checks in the constructor: a class can't just be called like a normal function; super must be called before interacting with this, etc. That results in both additional code in the constructor functions as well as some runtime overhead every time an Observable is instantiated.

Macil commented 7 years ago

Oh yeah, those could add up for Kefir.

mAAdhaTTah commented 7 years ago

Yeah. I do really like the idea of converting everything to ES6 classes. We would be able to get rid of the extends & inherits helpers, and I think it makes the codebase more idiomatic, but we could / should only do that in a world in which class does not need to be transpiled. So maybe we revisit this when IE11 get retired.