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

[BUG] not @computed decorators work in file.js but fail in file.ts #162

Closed
bartocc opened this Issue Aug 24, 2017 · 11 comments

Comments

Projects
None yet
3 participants
@bartocc
Contributor

bartocc commented Aug 24, 2017

ember-decorators version

yarn list v0.27.5
└─ ember-decorators@1.2.2
Done in 1.11s.

how to reproduce ?

=> you should get the followin JS error

ember.debug.js:19840 TypeError: Cannot read property 'enumerable' of undefined
    at handleDescriptor (decorator-wrappers.js:15)
    at decorator-wrappers.js:43
    at __decorate (component.js:68)
    at Module.callback (component.js:100)
    at Module.exports (loader.js:106)
    at requireModule (loader.js:27)
    at Class._extractDefaultExport (index.js:387)
    at Class.resolveOther (index.js:108)
    at Class.superWrapper [as resolveOther] (ember.debug.js:42856)
    at Class.resolve (ember.debug.js:7200)
  • stop your server
  • rename app/components/foo-bar/component.ts to component.js
  • start your server again

=> you should get no JS error and the following text rendered in the browser

I am foo bar 
B
@bartocc

This comment has been minimized.

Show comment
Hide comment
@bartocc

bartocc Aug 24, 2017

Contributor

@pzuraq took a stab at it and reported his findings in the following slack discussion

https://embercommunity.slack.com/archives/C64BW2L9X/p1503506282000634

Contributor

bartocc commented Aug 24, 2017

@pzuraq took a stab at it and reported his findings in the following slack discussion

https://embercommunity.slack.com/archives/C64BW2L9X/p1503506282000634

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Sep 7, 2017

Contributor

The slack archive has disappeared here so going to report my findings for posterity:

According to the decorators/class fields specs, class fields which are decorated will not actually have a descriptor on the class before the decorator is run. This makes sense as class fields are not prototype state but instance state - they aren't assigned until the constructor of the instance runs.

Babel is actually in the wrong here, and all of our decorators assume that a descriptor will exist and we can use its properties because that's how it currently transpiles it. Typescript has it right, we'll have to make decorators for class fields which don't have descriptors.

Contributor

pzuraq commented Sep 7, 2017

The slack archive has disappeared here so going to report my findings for posterity:

According to the decorators/class fields specs, class fields which are decorated will not actually have a descriptor on the class before the decorator is run. This makes sense as class fields are not prototype state but instance state - they aren't assigned until the constructor of the instance runs.

Babel is actually in the wrong here, and all of our decorators assume that a descriptor will exist and we can use its properties because that's how it currently transpiles it. Typescript has it right, we'll have to make decorators for class fields which don't have descriptors.

@bartocc

This comment has been minimized.

Show comment
Hide comment
@bartocc

bartocc Sep 7, 2017

Contributor

Should we also create a ticket on the babel repo ?

Contributor

bartocc commented Sep 7, 2017

Should we also create a ticket on the babel repo ?

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Sep 11, 2017

Contributor

I believe they're currently rewriting a lot of things, but yeah may be a good idea to open it so they're aware

Contributor

pzuraq commented Sep 11, 2017

I believe they're currently rewriting a lot of things, but yeah may be a good idea to open it so they're aware

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Sep 15, 2017

Contributor

Verified that this appears to be fixed on Babel 7, we should update and close this once it's released.

Contributor

pzuraq commented Sep 15, 2017

Verified that this appears to be fixed on Babel 7, we should update and close this once it's released.

@bartocc

This comment has been minimized.

Show comment
Hide comment
@bartocc

bartocc Sep 28, 2017

Contributor

@pzuraq if I understand all this correctly, when using Babel 7 or TypeScript, you get the same error, right ?

So ember-decorators still needs to handle situations where there are no descriptors.

Contributor

bartocc commented Sep 28, 2017

@pzuraq if I understand all this correctly, when using Babel 7 or TypeScript, you get the same error, right ?

So ember-decorators still needs to handle situations where there are no descriptors.

@theseyi

This comment has been minimized.

Show comment
Hide comment
@theseyi

theseyi Oct 2, 2017

That seems correct @bartocc, ran into this issue this morning with TypeScript. For class fields, TypeScript will pass the descriptor argument: desc as undefined. Ember computed decorators, for example @alias is created with decoratedPropertyWithRequiredParams, which further invokes decoratorWithParams. The returned closure checks if the last argument is a descriptor, and if not as it the case with class fields invokes handleDescriptor with the arguments object, the decorator function and any params.

The problem here is that handleDescriptor is the wrong function to call here since there is no descriptor but in the case of TypeScript, undefined the parameter is always undefined, which leads to the exception above.

theseyi commented Oct 2, 2017

That seems correct @bartocc, ran into this issue this morning with TypeScript. For class fields, TypeScript will pass the descriptor argument: desc as undefined. Ember computed decorators, for example @alias is created with decoratedPropertyWithRequiredParams, which further invokes decoratorWithParams. The returned closure checks if the last argument is a descriptor, and if not as it the case with class fields invokes handleDescriptor with the arguments object, the decorator function and any params.

The problem here is that handleDescriptor is the wrong function to call here since there is no descriptor but in the case of TypeScript, undefined the parameter is always undefined, which leads to the exception above.

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Oct 2, 2017

Contributor

Actually looking at more recent versions of the spec, it looks like both of them may need an update. It's hard to say exactly (spec language being notoriously difficult to read) but these are the two relevant sections:

  1. DecorateElement
  2. FromElementDescriptor

If I'm reading this correctly (and that's a big if) it looks like the decorator API is going to change to return quite a bit more information, and the signature of decorators will change to fn(elementObject) where elementObject contains all of that info.

In the end the spec is in flux so I don't think we can say for sure who is right here. For some of the decorators we have we can definitely account for there being no way to access the initializer, but for others we may need the value (and be dead in the water). Either way, we should fix the decorators for now and wait for the next revision of TS and Babel to try to sort it out.

Contributor

pzuraq commented Oct 2, 2017

Actually looking at more recent versions of the spec, it looks like both of them may need an update. It's hard to say exactly (spec language being notoriously difficult to read) but these are the two relevant sections:

  1. DecorateElement
  2. FromElementDescriptor

If I'm reading this correctly (and that's a big if) it looks like the decorator API is going to change to return quite a bit more information, and the signature of decorators will change to fn(elementObject) where elementObject contains all of that info.

In the end the spec is in flux so I don't think we can say for sure who is right here. For some of the decorators we have we can definitely account for there being no way to access the initializer, but for others we may need the value (and be dead in the water). Either way, we should fix the decorators for now and wait for the next revision of TS and Babel to try to sort it out.

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Oct 18, 2017

Contributor

Update on this:

The behavior differences between Typescript decorators and the Babel decorators that ember-decorators were designed for are larger than it seemed at first. Supporting TS now would likely mean having to rewrite a lot of the current logic, and some behavior simply may not work in TS since we don't have any way of accessing class field descriptors.

Babel is currently implementing the Stage 2 decorators spec, so we'll have to update either way once that lands. The TS design meeting notes indicate that they won't be updating anything until decorators reach stage 3, per their policy (only implement stage 3 features), so it could be a while 😕

Contributor

pzuraq commented Oct 18, 2017

Update on this:

The behavior differences between Typescript decorators and the Babel decorators that ember-decorators were designed for are larger than it seemed at first. Supporting TS now would likely mean having to rewrite a lot of the current logic, and some behavior simply may not work in TS since we don't have any way of accessing class field descriptors.

Babel is currently implementing the Stage 2 decorators spec, so we'll have to update either way once that lands. The TS design meeting notes indicate that they won't be updating anything until decorators reach stage 3, per their policy (only implement stage 3 features), so it could be a while 😕

@bartocc

This comment has been minimized.

Show comment
Hide comment
@bartocc

bartocc Oct 19, 2017

Contributor

thx for the update @pzuraq. I guess for the time being, using ember-decorators means not using ember-cli-typescript 😕

Contributor

bartocc commented Oct 19, 2017

thx for the update @pzuraq. I guess for the time being, using ember-decorators means not using ember-cli-typescript 😕

@pzuraq

This comment has been minimized.

Show comment
Hide comment
@pzuraq

pzuraq Feb 23, 2018

Contributor

v2.0.0-beta.1 has been released and should fix these issues! Let me know if you have any problems, I'll close this issue once we get to v2.0.0 stable

Contributor

pzuraq commented Feb 23, 2018

v2.0.0-beta.1 has been released and should fix these issues! Let me know if you have any problems, I'll close this issue once we get to v2.0.0 stable

@pzuraq pzuraq closed this May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment