Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Decorators considered attached to methods, or to property keys? #22

Open
loganfsmyth opened this issue May 31, 2015 · 8 comments
Open

Comments

@loganfsmyth
Copy link

In reading Babel yesterday and looking at this spec, there's an edge case that was unclear to me that I was curious about. Mostly around the expected execution order of things, and around which descriptor would be expected to be passed to the decorators.

This example uses a combination of getter and normal methods to avoid #10.

let prop = 'prop';

class Example {
    @decorator1
    get [prop](){

    }

    @decorator2
    prop(){

    }
}

Would the expectation be that both decorators would get a descriptor containing the final resolved Data descriptor, or would the first get an Accessor descriptor that would then be discarded, with the second descriptor getting the Data descriptor?

The farther edge case on this that I was curious about is if there were a third property at the end

@decorator3
set [prop](){}

with the currently proposed grouping of property accessors, what would the final value be, and what is passed to the descriptors?

I guess this comes down to a question of, are decorators associated with a property name, and they get applied after the class properties have all be flattened into a standard list of descriptors, or are they attached to a particular method, with overrides to that method from computed properties essentially nullifying the decorator's effect.

I don't know why'd actually write a class like this, but I was curious since it seemed non-obvious.

Grouping them by property key makes the get/set grouping happen automatically, but it seems like it could lead to confusion if for some reason you did have computed properties that overrode declared properties.

Sorry if this has already been answered elsewhere!

@dead-claudia
Copy link

I'm wanting to say methods, since it gets the latest method implementation off of Object.getOwnPropertyDescriptor, and that descriptor can't be mutated by another Object.defineProperty. You override the method, you override the whole implementation, descriptor and all.

@loganfsmyth
Copy link
Author

@IMPinball Care to clarify a bit? Part of my question simplifies down to when does it

get the latest method implementation off Object.getOwnPropertyDescriptor

actually happen? You can do that after evaluating each property after 14.5.14 step #21 but at that point it's hard to know which decorators should actually be applied (at least if decorators are attached to specific methods, not property names) because then a computed property may have redefined the property to be something else.

@dead-claudia
Copy link

Okay. Sorry for the very poor phrasing. I was a bit tired at the time. It basically does this:

class Foo {
  @decorate bar() {
    return something();
  }
}

// translates to

class Foo {}

let bar = {
  configurable: true,
  enumerable: false,
  writable: true,
  value() {
    something();
  },
};

let temp = decorate(Foo.protoype, 'bar', bar);
if (typeof temp !== 'undefined') bar = temp;
Object.defineProperty(Foo.prototype, 'bar', bar);

For getters and setters, it works like this:

class Foo {
  @decorate
  get bar() {
    return something();
  }

  @decorate
  set bar(value) {
    return somethingElse(value);
  }
}

// translates to

class Foo {}

let bar = {
  configurable: true,
  enumerable: false,
  writable: true,
  get() {
    something();
  },
  set(value) {
    somethingElse(value);
  },
};

let temp = decorate(Foo.protoype, 'bar', bar);
if (typeof temp !== 'undefined') bar = temp;
Object.defineProperty(Foo.prototype, 'bar', bar);

For a decorated getter + undecorated setter, it's like this:

class Foo {
  @decorate
  get bar() {
    return something();
  }

  set bar(value) {
    return somethingElse(value);
  }
}

// translates to

class Foo {}

let bar = {
  configurable: true,
  enumerable: false,
  writable: true,
  get() {
    something();
  },
};

let temp = decorate(Foo.protoype, 'bar', bar);
if (typeof temp !== 'undefined') bar = temp;
Object.defineProperty(Foo.prototype, 'bar', bar);

In your specific case, it would work like this:

class Foo {
  @decorate1
  get [prop]() {
    return doA();
  }

  @decorate2
  prop() {
    return doB();
  }

  @decorate3
  set [prop](value) {
    return doC(value);
  }
}

// translates to

class Foo {}

// This cannot be mutated by descriptors when they are applied
let key = prop;

let computedGet = {
  configurable: true,
  enumerable: false,
  writable: true,
  get() {
    doA();
  },
};

let temp = decorate1(Foo.protoype, key, computedGet);
if (typeof temp !== 'undefined') computedGet = temp;
Object.defineProperty(Foo.prototype, key, computedGet);

let constant1 = {
  configurable: true,
  enumerable: false,
  writable: true,
  value() {
    doB();
  },
};

temp = decorate2(Foo.protoype, 'prop', constant1);
if (typeof temp !== 'undefined') constant1 = temp;
Object.defineProperty(Foo.prototype, 'prop', constant1);

// It could've been mutated already
key = prop
let computedSet = {
  configurable: true,
  enumerable: false,
  writable: true,
  set(value) {
    doC(value);
  },
};

let temp = decorate3(Foo.protoype, key, computedSet);
if (typeof temp !== 'undefined') computedSet = temp;
Object.defineProperty(Foo.prototype, key, computedSet);

Should this help?

Also, if I'm wrong about this desugaring, feel free to correct me.


// Another way to write this, pulling the common part out into a function

function _applyDecorator(decorator, host, prop, desc) {
  Object.assign(desc, {
    enumerable: true,
    configurable: false,
    writable: true,
  });

  let tmp = decorator(host, prop, desc)
  if (typeof tmp !== 'undefined') desc = tmp;
  Object.defineProperty(host, prop, desc);
}

/*
class Foo {
  @decorate1
  get [prop]() {
    return doA();
  }

  @decorate2
  prop() {
    return doB();
  }

  @decorate3
  set [prop](value) {
    return doC(value);
  }
}
*/

class Foo {}

_applyDescriptor(decorate1, Foo.prototype, prop, {
  get() {
    doA();
  },
});

_applyDecorator(decorate2, Foo.prototype, 'prop', {
  value() {
    doB();
  },
});

_applyDescriptor(decorate3, Foo.prototype, prop, {
  set(value) {
    doC(value);
  },
});

// For a combined getter/setter like below:
/*
class Foo {
  @decorate
  get bar() {
    return something();
  }

  @decorate
  set bar(value) {
    return somethingElse(value);
  }
}
*/

class Foo {}

_applyDescriptor(decorate, Foo.prototype, bar, {
  get() {
    something();
  },
  set(value) {
    somethingElse(value);
  },
});

@loganfsmyth
Copy link
Author

Perfect, thanks for the examples. So to take from that, this discrepancy is what I'm concerned about:

/*
class Foo {
  @decorate
  get bar() {
    return something();
  }

  @decorate
  set bar(value) {
    return somethingElse(value);
  }
}
*/

class Foo {}

_applyDescriptor(decorate, Foo.prototype, 'bar', {
  get() {
    something();
  },
  set(value) {
    somethingElse(value);
  },
});

vs

/*
var bar = 'bar';
class Foo {
  @decorate
  get [bar]() {
    return something();
  }

  @decorate
  set [bar](value) {
    return somethingElse(value);
  }
}
*/

class Foo {}

_applyDescriptor(decorate, Foo.prototype, bar, {
  get() {
    something();
  }
});

_applyDescriptor(decorate, Foo.prototype, bar, {
  set(value) {
    somethingElse(value);
  },
});

At least given your examples, it seems like changing a property from static to computed would cause the descriptor change from being a combination of get/set to two separate calls with one having get and another having set.

@dead-claudia
Copy link

To be honest, though, this just comes down to another ambiguity, just like #10, but a semantic one instead of a syntactic one. @wycats PTAL

@loganfsmyth
Copy link
Author

Yup, that's why I was curious about it. This example does a good job of exposing my main concern, which is around this apparent auto-grouping of getter/setter pairs. It makes me wonder if things wouldn't be better off if the decorators were processed after the initial defineProperty calls, then it'd desugar to more like

class Foo {
  @decorate
  get bar() {
    return something();
  }

  set bar(value) {
    return somethingElse(value);
  }
}

// translates to

class Foo {
  get bar() {
    return something();
  }

  set bar(value) {
    return somethingElse(value);
  }
}

let descriptor = Object.getOwnPropertyDescriptor(Foo.prototype, 'bar');
let temp = decorate(Foo.protoype, 'bar', descriptor);
if (typeof temp !== 'undefined') descriptor = temp;
Object.defineProperty(Foo.prototype, 'bar', descriptor);

That conceptually makes the getter/setter grouping logic simpler, but it'd potentially need some extra work to take multiple computed properties overlapping eachother.

@dead-claudia
Copy link

And this is why I prefer doing things like this (the common case is just wrapping descriptor.value):

function wrap(f) {
  return function (x) {
    doSomething(x);
    return f.call(this);
  }
}

const Foo = {
  __proto__: Bar, // inherit
  create() {
    return Object.create(Foo);
  },
  method() {
    doA();
  },
  wrap: wrap(function () {
    doB();
  },
  get prop() {
    return c;
  },
  set prop(value) {
    return d = value;
  },
};

I needed almost 0 syntax sugar for that. I used ES6 __proto__ and shorthand methods. If I wanted to transpile this to get rid of the ES6 features, I could simply walk the AST and output this:

var Foo = _.assign(Object.create(Bar), {
  create: function () {
    return Object.create(Foo);
  },
  method: function () {
    doA();
  },
  wrap: wrap(function () {
    doB();
  },
  get prop() {
    return c;
  },
  set prop(value) {
    return d = value;
  },
});

Or, I could add a utility function and transpile to this:

function type(super, obj) {
  if (arguments.length === 0) {
    throw new TypeError();
  } else if (arguments.length === 1) {
    return obj;
  } else {
    var ret = Object.create(super);
    for (var prop in obj) if ({}.hasOwnProperty.call(obj, prop)) {
      Object.defineProperty(ret, prop, Object.getOwnPropertyDescriptor(obj, prop));
    }
    return ret;
  }
}

var Foo = type(Bar, {
  create: function () {
    return Object.create(Foo);
  },
  method: function () {
    doA();
  },
  wrap: wrap(function () {
    doB();
  },
  get prop() {
    return c;
  },
  set prop(value) {
    return d = value;
  },
});

So simple. So clean. And this is why I like prototypal inheritance.

@dead-claudia
Copy link

Thing is, I'm not sure if that's the correct behavior, though. So that's
why I pingged wycats.

On Tue, Jun 23, 2015, 20:33 Logan Smyth [email protected] wrote:

Yup, that's why I was curious about it. This example does a good job of
exposing my main concern, which is around this apparent auto-grouping of
getter/setter pairs. It makes me wonder if things wouldn't be better off if
the decorators were processed after the initial defineProperty calls, then
it'd desugar to more like

class Foo {
@decorate
get bar() {
return something();
}

set bar(value) {
return somethingElse(value);
}
}

// translates to

class Foo {
get bar() {
return something();
}

set bar(value) {
return somethingElse(value);
}
}

let descriptor = Object.getOwnPropertyDescriptor(Foo.prototype, 'bar');
let temp = decorate(Foo.protoype, 'bar', descriptor);
if (typeof temp !== 'undefined') descriptor = temp;
Object.defineProperty(Foo.prototype, 'bar', descriptor);

That conceptually makes the getter/setter grouping logic simpler, but it'd
potentially need some extra work to take multiple computed properties
overlapping eachother.


Reply to this email directly or view it on GitHub
#22 (comment)
.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants