The Wayback Machine - https://web.archive.org/web/20191103234322/https://github.com/microsoft/TypeScript/issues/32395
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

Property decorator documentation is inaccurate? #32395

Open
NetanelBasal opened this issue Jul 14, 2019 · 7 comments
Labels

Comments

@NetanelBasal
Copy link

@NetanelBasal NetanelBasal commented Jul 14, 2019

From the official docs about property decorators:

"The return value is ignored too."

But the following example returns a new descriptor, and it works fine:

class Greeter {
  @logProperty
  greeting;
}

function logProperty(target, key): any {
    var value;

    var getter = function() {
      console.log('getter');
      return value;
    };

    var setter = function(newVal) {
      value = newVal;
    };

    return {
      get: getter,
      set: setter,
      enumerable: true,
      configurable: true
    };
}

const g = new Greeter();
const b = g.greeting;
@fatcerberus

This comment has been minimized.

Copy link

@fatcerberus fatcerberus commented Jul 14, 2019

var __decorate = (this && this.__decorate) || function(decorators, target, key, desc) {
    var c = arguments.length,
        r = c < 3 ? target : desc === null ? desc = Object.getOwnPropertyDescriptor(target, key) : desc,
        d;
    if (typeof Reflect === "object" && typeof Reflect.decorate === "function") r = Reflect.decorate(decorators, target, key, desc);
    else
        for (var i = decorators.length - 1; i >= 0; i--)
            if (d = decorators[i]) r = (c < 3 ? d(r) : c > 3 ? d(target, key, r) : d(target, key)) || r;
    return c > 3 && r && Object.defineProperty(target, key, r), r;
};

The return value is indeed not ignored here, and is in fact used as input for a call to Object.defineProperty(). I think the documentation is inaccurate.

However, a potential pitfall I see: the decorated property ends up being defined on the prototype and is no longer an instance property because:

__decorate([
    logProperty
], Greeter.prototype, "greeting", void 0);

If you add an initializer to the greeting property (greeting = 1;) it ends up calling the setter as a result, because the compiler moves the initialization to the constructor.

@NetanelBasal

This comment has been minimized.

Copy link
Author

@NetanelBasal NetanelBasal commented Jul 14, 2019

Yes, I'm working on a new article which explains how it works under the hood. That's how I found this behavior.

I don't think it's a pitfall. It seems like the intended behavior.

@fatcerberus

This comment has been minimized.

Copy link

@fatcerberus fatcerberus commented Jul 14, 2019

I meant "pitfall" in the sense of being a footgun, people might not expect instance properties to get moved to the prototype just because they added a decorator 😉

@NetanelBasal

This comment has been minimized.

Copy link
Author

@NetanelBasal NetanelBasal commented Jul 14, 2019

It's been added to the prototype anyway:

__decorate([
  logProperty
], Greeter.prototype <=====, "greeting", void 0);
@fatcerberus

This comment has been minimized.

Copy link

@fatcerberus fatcerberus commented Jul 14, 2019

Right, that's what I said - it only goes to the prototype if you have a decorator, otherwise it's strictly an own property of the instance. Seemed surprising, even if it's intentional.

@NetanelBasal

This comment has been minimized.

Copy link
Author

@NetanelBasal NetanelBasal commented Jul 14, 2019

Agree. They explain why in the docs:

"This is because there is currently no mechanism to describe an instance property when defining members of a prototype, and no way to observe or modify the initializer for a property. "

@NetanelBasal NetanelBasal changed the title Property decorator documentation is misleading? Property decorator documentation is inaccurate? Jul 14, 2019
@d3lm

This comment has been minimized.

Copy link

@d3lm d3lm commented Jul 16, 2019

Great find @NetanelBasal 👏

@RyanCavanaugh RyanCavanaugh added the Docs label Jul 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.