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

Initialize class property that has no initializer with this.prop #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

shuhei
Copy link

@shuhei shuhei commented Dec 23, 2015

Currently class properties with no initializer are set as null and un-writable, which is problematic especially for Angular 2 apps that utilize decorators often.

class SomeComponent {
  @Input() prop;
}

According to the ES Class Fields and Static Properties proposal, no initializer should be treated as an initializer with this.prop.

@shuhei
Copy link
Author

shuhei commented Dec 23, 2015

@loganfsmyth I'm still not sure about this line.

const property = t.isLiteral(node.key) ? node.key : t.stringLiteral(node.key.name);

Does it allow string/number literals to be class properties like the following? I tried them but babel raised errors.

class Foo {
  @dec
  123;

  @dec
  "hello";
}

@loganfsmyth
Copy link
Owner

Eww, are you really monkey patching Babylon? That's so fragile, why would you do that... If it's not on a standards track somewhere, monkey-patching it in and telling people to use it is just going to cause pain for your users in the future :(

The objective of this module is the emulate the existing behavior of Babel 5. What does Babel 5 do in this case? My current position is that anything that wasn't in Babel 5 should wait until the new revision of the decorators spec is available.

The const property applies to all class method/prop nodes. In the case of class properties, the parser only supports parsing identifiers in that location.

@shuhei
Copy link
Author

shuhei commented Dec 29, 2015

@loganfsmyth I have given up the parameter decorator support and will stop monkey-patching in the repo.

Anyway, as far as I researched, Babel 5 works as same as this plugin does. It sets a decorated property with no initializer undefined and makes it un-writable.

function dec() {
}

class Foo {
  @dec withDec;
  withoutDec;
}
Foo.prototype.withDec = 'with';
Foo.prototype.withoutDec = 'without';

var foo = new Foo();
console.log(foo.withDec); // undefined (should be 'with')
console.log(Object.getOwnPropertyDescriptor(foo, 'withDec')); // { value: undefined, writable: false, enumerable: true, configurable: true } (should be writable)
console.log(foo.withoutDec); // without
console.log(Object.getOwnPropertyDescriptor(foo, 'withoutDec')); // undefined (should be defined)

I understand that this plugin is for emulating the behavior of Babel 5's decorator support. But it's not only about decorators here. It messes up the behavior of class properties.

Which way do you think I should take? Should I make a PR to the babel repo and come back here when it's merged?

@mlegenhausen
Copy link

What will be probably the future behaviour of decorators? Will they be un-writable?

@shuhei
Copy link
Author

shuhei commented Jan 6, 2016

According to the proposal, it should be same as @Input() prop = this.prop; and of course not un-writable.

Actually there two separate issues.

  1. decorated property with no initializer gets un-writable
  2. (the proposal) no initializer should be same as this.prop

@mlegenhausen
Copy link

Ok thanks. Currently it is really annoying to set default values everywhere to get the appropriate behaviour especially as you said when you want to work with angular2.

Anyway, a changelog would be nice it took me some time to find this fundamental change in behavior.

@sonicoder86
Copy link

Is it only pending on a changelog?

@jshthornton
Copy link

jshthornton commented Mar 25, 2017

Any update on this? It breaks @lazyInject from Inversify
I have temporarily bypassed it by doing this:

  constructor(props) {
    super(props);

    lazyInject(tokens.ProductActionContainer)(this.__proto__, 'ProductActionContainer');
  }

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

Successfully merging this pull request may close these issues.

6 participants