-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Decorators 2 Transform [WIP] #6107
Conversation
I accidentally opened this PR with incomplete info. Now that I've edited it to include all details, I'm reopening |
I'm proposing some changes to the decorator proposal at https://github.com/littledan/proposal-unified-class-features . You can see a summary of the changes in this presentation. The committee and decorators champions seemed to view it positively. I'd be interested to hear what you think. I don't think this follow-on proposal should affect most of the implementation. I'll review this patch as an implementation of this decorators proposal. |
@littledan Does removing finishers from element decorators mean they can no longer safely attach meta data? Especially if finishers can now replace the class completely? I thought this was done explicitly to allow meta data "annotation"-style decorators that are hard or impossible to implement with stage 0? |
@jkrems You can attach metadata by putting it on the methods in an additional property, or making a WeakMap keyed off of the method. Would that work for you? |
@littledan The nice thing about finalizers was that it allowed to the 2nd thing safely - no matter what other decorators would do. @injectDataSource
class MyResource {
@deprecated // not 100% sure about evaluation order
@GET('/users/{id}') // wants to attach meta data to `getUser`
async getUser(params) { /* ... */ }
} We're assuming here that:
In the previous stage 2 iteration, |
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 just looked at the tests for now; will look at the code later
function log(message) { | ||
return function (descriptor) { | ||
let oldFunc = descriptor.value; | ||
descriptor.value = function(...args) { |
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.
Somewhat unfortunately, there is no way of making this function's name derive from the original function name (instead of "value") other than by calling defineProperty
on it afterwards 🤔
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.
Actually, we'll probably be passing the element descriptor to the decorator and not the property descriptor. I just had a doubt on how that'd work when you change the key of the descriptor you've returned and that's why I made the call to implement it like this. Once I get a little clarification on assumption #4 from @littledan or someone, I'll make the desired changes
} | ||
|
||
class Foo { | ||
@overrider(() => 3) method() { |
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.
What is this
here? Is it the constructor, the outer this
, or the newly created instance?
Could be worth having a test for it.
const clone = { | ||
kind: "property", | ||
isStatic: !!isStatic, | ||
key: name, |
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.
What should happen if the name conflicts? @spare("foo") foo() {}
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.
Currently what I've interpreted from the spec is that it should "merge". By that I mean it will just take on the value of the last property added with the key "foo". See decorateElement step 6. Also see mergeDuplicateElements
helper
|
||
undecorated() {} | ||
|
||
}, [["undecorated"]], [["method", [methDec]]], void 0)([classDec]); |
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.
(Having not looked at the decorate implementation yet) Too many arrays? Some of these look like they could be flattened one level, but I'm only guessing at the meaning of each parameter.
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.
currently I'm using const [key, isStatic] = [..]
so that's why the inner items of those are arrays (in case of a non static member, I just omit the second array element). We could certainly simplify it further if needed, e.g. if it's static then it'll look like ["method", true]
otherwise it'll look like just "method"
and not ["method"]
|
||
[calculated + and + undecorated]() {} | ||
|
||
}, [["andAnUndecoratedMethod"], [calculated + and + undecorated]], [["m1", [dec]], ["m2", [bar, foo.bar(baz)]], [(_key = 3 + 7), [dec], true]], void 0)([decorator]); |
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.
calculated + and + undecorated
should be put in a ref as well (could be an impure expression)
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'm using babel's built-in isPure
which is why that wasn't put in a ref
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.
You're right, this should have been put in a ref. Nicolo pointed out the error in code and I've fixed it, will push soon
} | ||
|
||
// decorate and store in elementDescriptors | ||
for (const [key, decorators, isStatic] of memberDecorators) { |
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.
Can we ES2015 here?
And a bit later, Array.from
will require a polyfill.
It could cause issues like this #5876, right?
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 wish we could. But if we can't, I'll refactor it to use ES5, it shouldn't be complex. I'll leave this be till the end though, since the code is functionally correct so for the review we could focus on just that
@@ -21,6 +21,6 @@ function t() { | |||
|
|||
function t() { | |||
for (var i = 0; i < arguments.length; i++) { | |||
return arguments.length <= i ? undefined : arguments[i]; |
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.
this should be fixed
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'm not sure if this is related to this PR. I'll investigate more
for (const [key, decorators, isStatic] of memberDecorators) { | ||
const target = isStatic ? constructor : prototype; | ||
const propertyDescriptor = | ||
elementDescriptors.has([key, isStatic]) && elementDescriptors.get([key, isStatic]).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.
This has call will always return false, since ES6 maps are by identity. Since [1, 2] !== [1, 2], calling Map.prototype.has on a fresh array will never do what you're hoping for.
Instead, you could maintain two Sets of keys, or two objects with keys and values, for the static and non-static halves.
|
||
finishers = finishers.concat(result.finishers); | ||
//TODO: heritage hacks so result.constructor has the correct prototype and instanceof results | ||
//TODO: step 38 and 39, what do they mean "initialize"? |
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.
"Initialize" was about having methods and the constructor start in some state where they would be unusable, and make them usable later. In my follow-on decorators proposal, I'm suggesting to get rid of this initialized/uninitialized state. I think it's OK to go without it to start, even if it's possible that TC39 will ask to bring it back.
); | ||
|
||
finishers = finishers.concat(result.finishers); | ||
//TODO: heritage hacks so result.constructor has the correct prototype and instanceof results |
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.
Heritage is just given as an argument to class decorators; it's the responsibility of the decorators to do the appropriate thing with that, I believe.
And other review suggestions
Apologies for responding late to the review. Uni workload has started again. Updates: Now we're merging getters and setters & other things pointed out by littledan in their review Todo:
|
I had missed extends in the test case but the asserts were expecting it. I thought that tests were failing for more sinister reasons, but thankfully it isn't so
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've confirmed with @bterlson and @wycats that http://tc39.github.io/proposal-unified-class-features should be considered to subsume http://tc39.github.io/proposal-decorators for the planned spec text.
Update: @nicolo-ribaudo asked me if they could take this up, and I told them they could. The outstanding work on this PR would have been the todo in my previous comment and then any changes between the spec at the time this PR was made and the spec now (I'm not sure about the exact status of this, seems like http://tc39.github.io/proposal-unified-class-features that @littledan's comment specified is dead and http://tc39.github.io/proposal-decorators has been updated) Since Babel's codebase has changed much (and the proposal may have too), I guess Nicolo's taking the route of reworking this feature and salvaging code from this PR as needed in #7542 . Kudos and good luck! Here's what might be useful from this PR:
|
Note: I'll use the terms evaluation to mean evaluation of
@dec(args)
and decoration to mean actual execution of the decoratorsAlso, note that the decorator proposal introduces the concept of an elementDescriptor, which is distinct from the propertyDescriptor we all know about.
To understand the code, you need to know that an elementDescriptor is an object with the shape:
I've implemented the methods
decorate
,decorateElement
,decorateClass
,makeElementDescriptor
, andmergeDuplicateElements
inhelpers.js
. Out of these,decorateElement
anddecorateClass
correspond to the spec operations DecorateElement, and DecorateClass respectively.mergeDuplicateElements
corrseponds to the operation used in step 6 of both DecorateClass and DecorateElement.The method
decorate
runs the whole thing. It takes care of the evaluation order (see below) and corrsponds to Runtime Semantics: ClassDefinitionEvaluation step 21 onwards.The changes in babel-core are from #6058
Assumptions Made: (Please feel free to correct)
a. evaluate all method decorators in order [step 21.g. of above spec link] (left to right / outer to inner)
b. (decoration) execute all method decorators in reverse order step 24.a.(right to left / inner to outer)
c. evaluate all class decorators in order step 29. (assuming that DecoratorListEvaluation happens just before this, since it's not explicitly mentioned)
d. (decoration) execute all class decorators in reverse order step 29
finisher
attached to the elementDescriptor and explicitly returning finishers array. I've largely ignored the former because it doesn't seem to make any sense.elementDescriptor
is passed to method decorators and notpropertyDescriptor
, and that's how I had implemented it in the past. However, I changed it to usepropertyDescriptor
because usingelementDescriptor
would imply that the decorator can change the key of the method being decorated, and if it is static or not. I'm not sure if this behaviour is intended or unintended. I need more feedback on this from the spec designers. Changing back toelementDescriptor
shouldn't be too much trouble.TODO / WIP: