tc39 / proposal-class-public-fields

Stage 2 proposal for public class fields in ECMAScript
https://tc39.github.io/proposal-class-public-fields/
488 stars 25 forks source link

extend to get/set methods? #32

Closed KiaraGrouwstra closed 8 years ago

KiaraGrouwstra commented 8 years ago

I've been happily trying this proposal through babel, and appreciate the convenience it adds for the purpose of assigning methods -- unlike the traditional way of adding class methods, this allows using wrapper functions around methods more easily (i.e. without like moving the assignments into the constructor).

As it stands, it does not appear possible to similarly assign set/get methods though. I'd be happy to be able to see this become available for that purpose as well. To me this feels like a natural extension to the functionality provided by the current proposal, though I don't currently see anything related to this mentioned, I imagine likely because intent has been different.

This may be outside of scope here, and perhaps Yehuda Katz's decorators proposal may be seen as having overlap in the sense of allowing one to decorate methods. In any case, I'd be glad to leave this here for further discussion.

ljharb commented 8 years ago

@tycho01 you mean like:

class Foo {
  get foo() {}
  set bar(v) {}
}

which is already part of ES6/ES2015?

KiaraGrouwstra commented 8 years ago

Hi @ljharb, Sorry I hadn't added examples. As you said, that's already part of ES6.

ES6 methods went like this:

class Foo {
  bar() {}
}

This current proposal allows:

class Foo {
  bar = function() {} // or arrow function
}

Which is cool cuz it enables this:

class Foo {
  bar = my_wrapper(function() {})
}

And although ES6 has this:

class Foo {
  get foo() {}
  set bar(v) {}
}

What is not yet possible currently, is this:

class Foo {
  get foo = function() {} // or arrow functions
  set bar = function(v) {}
}

Which might also enable this:

class Foo {
  get foo = my_wrapper(function() {})
  set bar = my_wrapper(function(v) {})
}

I don't know if demand for this is substantial, but I'd just tried actually using this in my code, only to find it wasn't currently possible.

Edit: an obvious advantage of allowing set/get methods to be assigned is allowing reuse of logic, which may be relevant here since set/get methods might be somewhat prone to boilerplate code. Wrappers currently aren't an option there either, and while in the current use-case of the original proposal, assignments could just take place in the constructor instead, for get/set methods, I'm not sure how a similar alternative might work. But that might just be my ignorance as to what they compile down to.

ljharb commented 8 years ago

I feel like in this case you might do:

@getter
@my_wrapper
foo = function () {}

with the decorators proposal?

KiaraGrouwstra commented 8 years ago

Interesting. Yeah, that may well do it. :) I wouldn't mind either way, so yeah, I guess we can consider this closed.

KiaraGrouwstra commented 8 years ago

Out of curiosity, are you aware if anyone had pulled off @getter/@setter already? I couldn't find existing ones, so tried implementing them:

import { decorate } from 'core-decorators/lib/private/utils';

let decMethod = (k) => (target, key, descriptor, pars) => {
  const fn = descriptor.value;
  if (typeof fn !== 'function') {
    throw new SyntaxError(`can only decorate methods, while ${key} is a ${typeof fn}!`);
  }
  return {
    ...descriptor,
    [k]: fn.bind(this, ...arguments),
  };
}

export let getter = (...args) => decorate(decMethod('get'), args);

export let setter = (...args) => decorate(decMethod('set'), args);

I guess the implementation doesn't matter much, because it seems to have trouble parsing invocation already. While with similar approaches I could decorate methods just fine, this didn't work, I'm thinking because it ends up confused with the existence of two identically named methods (if one to be used as a setter, another as getter).

ljharb commented 8 years ago

@tycho01 your new descriptor still contains "value" - try instead of using object spread, explicitly grabbing the properties you want off of descriptor?

KiaraGrouwstra commented 8 years ago

Thanks, that'll teach me for borrowing bits of code I didn't fully understand the original purpose of. I tried just commenting that bit, and it seems separately the getter/setter decorators are executing without error. It seems the actual issue now is when you use both, and thus have two identically-named methods (if decorated with @getter/@setter), info of the first instance goes lost. It looks like that key isn't actually set from this function though, so it looks like addressing that from here might not be that easy (from my limited knowledge). Thanks though.