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 misc fixes #14339
Decorators misc fixes #14339
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51444/ |
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.
LGTM pending the message update. I'll mark this as "spec compliance", since we usually use that when we make a transform more restrictive to match the spec (since there is a very small chance that it will break someone, if they are using it in a non-spec-compliant way).
683d456
to
e32c532
Compare
@@ -306,7 +317,7 @@ function applyMemberDec( | |||
if (kind === 0 /* FIELD */) { | |||
initializer = newValue; | |||
} else if (kind === 1 /* ACCESSOR */) { | |||
initializer = newValue.initializer; | |||
initializer = newValue.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.
Since we are renaming a property here, I wonder if it makes sense to also support the old one with a warning:
if ((initializer = newValue.init) == null && (initializer = newValue.initializer) && typeof console !== "undefined") {
console.warn(".initializer has been renamed to .init as of March 2022")
}
@pzuraq This idea mostly came up while I was reviewing #14353, since it's not just a "we implemented the spec wrongly" anymore but a "the proposal had a change that made all the accessor initializers usages invalid".
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.
that seems reasonable to me, will update
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.
Also, feel free to review/approve this PR since you are the "expert in the field" for decorators! However, I'd like to wait merging this until #14353 is ready so that they can go in the same release.
This PR is good as long as it's followed shortly with the changes in #14353 |
Please review this PR by commits. Notable changes:
context.addInitializer
are now invoked without any params, previously we passed down the class instance or the decorated class.initialize
returned from the accessor decorator is now respected, previously we accessed.initializer
.TypeError
per spec.