-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Fix decorator in typescript targeting ES3 #343
Conversation
Fiddle, representing the problem: https://jsfiddle.net/Sl1v3r/r3oLj9fz/ |
@@ -48,6 +48,11 @@ export function createClassPropertyDecorator( | |||
} | |||
} | |||
}; | |||
if (arguments.length < 3) { | |||
// Typescript target is ES3, so it won't define property for us | |||
Object.defineProperty(target, key, descriptor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @andykog, what will this fix? If target is ES3, then Object.defineProperty
should not be available in the first place right? I guess people should just not compile for ES3. So maybe we should just throw an exception here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean some people can forget to specify typescript target, i thought, it would be nice if mobx still works. Fixing jsfiddle is also nice.
And it fixes Reflect.decorate (#333)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ha sorry yeah read this in the wrong other :) Thanks for sorting this out!
I'll run some more tests but I think this will do.
Op ma 20 jun. 2016 om 16:32 schreef Andy Kogut notifications@github.com:
In src/utils/decorators.ts
#343 (comment):@@ -48,6 +48,11 @@ export function createClassPropertyDecorator(
}
}
};
if (arguments.length < 3) {
// Typescript target is ES3, so it won't define property for us
Object.defineProperty(target, key, descriptor);
I mean some people can forget to specify typescript target, i thought, it
would be nice if mobx still works. Fixing jsfiddle is also nice.
And it fixes Reflect.decorate (#333
#333)—
You are receiving this because you commented.Reply to this email directly, view it on GitHub
https://github.com/mobxjs/mobx/pull/343/files/2f55b6b7de579e7aa3c2a5ca2f552f2c827eb0b0#r67699836,
or mute the thread
https://github.com/notifications/unsubscribe/ABvGhPWGhpfY_7NxHfvaK6O_iyrYXiFPks5qNqSGgaJpZM4I5MzZ
.
Thanks again for sorting this out! |
When tsc target is set to ES3 (default), it won't add 3'rd argument for decorator https://github.com/Microsoft/TypeScript/blob/v1.8.10/src/compiler/emitter.ts#L5707
Then it won't use Object.defineProperty https://github.com/Microsoft/TypeScript/blob/v1.8.10/src/compiler/emitter.ts#L364
I don't know, whether we should care about this setup, because mobx itself is not usable under ES3 environment, but this fixes problem on updating from mobx 2.2.2