Update decorators to match latest spec#14353
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51511/ |
| } | ||
|
|
||
| expect(() => { | ||
| getMetadata(metadataSymbol); |
There was a problem hiding this comment.
Will it throw if getMetadata is called in a static field decorator? If it throws then maybe we'd better move the test to a static field decorator so we are more confident that the context method is not invokable after the method decorators are applied.
There was a problem hiding this comment.
It should throw in the very next decorator, so we could apply a second decorator to the same element and have it call it
|
Ignore the codecov failure, it will give an higher result when merged to |
cd8f7b7 to
1c57fcf
Compare
| newClass = nextNewClass; | ||
| } | ||
|
|
||
| decorationState.finished = true; |
There was a problem hiding this comment.
Q: Should we also mark state as finished if applying decorator results to an abrupt completion?
I think we should, so the leaked context method will not be allowed to call again in a catch clause outside the class, but of course one can still use try-catch within the decorator.
There was a problem hiding this comment.
I actually think I need to update the spec to do this too, thanks for calling this out
- Update `initialize` -> `init` - Update decorator application ordering: 1. Static method decorators 2. Proto method decorators 3. Static field decorators 4. Proto field decorators - Throw errors when `addInitializer` or metadata methods are called outside of decoration
5236 -> 5003 bytes (after terser)
size: 5024 -> 5048
1c57fcf to
b00d11d
Compare
|
I'd like to merge this soonish, since I'd like to release the fix for #14357 and the other decorators PR has already been merged. I implemented all @JLHwung's suggestions, including #14353 (comment): even if it's a super edge case, it's what seems to be most coherent from a language design perspective. |
JLHwung
left a comment
There was a problem hiding this comment.
Now that calling context.addInitializer after decoration is an error, do we still need
initializers = initializers.slice();in pushInitializers? The leaked context method test was previously constructed for that.
| var decoratorFinishedRef = { v: false }; | ||
|
|
||
| try { | ||
| var ctx = Object.assign( |
There was a problem hiding this comment.
Not related to this PR: Object.assign is ES2015, we may need to import helpers.extends.
|
Oh good catch, |
| initializers[i].call(instance); | ||
| } | ||
| return instance; | ||
| }); |
There was a problem hiding this comment.
The pushInitializer variant in applyClassDesc can be simplified, too.
a91a39c to
c2bf48f
Compare
|
I deduped some more code, we are now at 4809 bytes after terser with vars managling enabled. |
nicolo-ribaudo
left a comment
There was a problem hiding this comment.
I made a bunch of minor helper refactors, but this is ready! I'll bump the helper version when releasing.
initialize->initaddInitializeror metadata methods are called outside of decorationIncludes changes from #14339