Skip to content
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

Merged
merged 9 commits into from Mar 14, 2022
Merged

Decorators misc fixes #14339

merged 9 commits into from Mar 14, 2022

Conversation

JLHwung
Copy link
Contributor

@JLHwung JLHwung commented Mar 8, 2022

Q                       A
Fixed Issues? Several decorators spec-compliance issues
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes?
License MIT

Please review this PR by commits. Notable changes:

  1. the initializer accepted by context.addInitializer are now invoked without any params, previously we passed down the class instance or the decorated class.
  2. the initialize returned from the accessor decorator is now respected, previously we accessed .initializer.
  3. Some errors are now constructed from the TypeError per spec.

@JLHwung JLHwung added PR: Bug Fix 🐛 A type of pull request used for our changelog categories Spec: Decorators labels Mar 8, 2022
@babel-bot
Copy link
Collaborator

babel-bot commented Mar 8, 2022

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51444/

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a 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).

@nicolo-ribaudo nicolo-ribaudo added PR: Spec Compliance 👓 A type of pull request used for our changelog categories and removed PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Mar 9, 2022
@@ -306,7 +317,7 @@ function applyMemberDec(
if (kind === 0 /* FIELD */) {
initializer = newValue;
} else if (kind === 1 /* ACCESSOR */) {
initializer = newValue.initializer;
initializer = newValue.initialize;
Copy link
Member

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".

Copy link
Contributor

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

Copy link
Member

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.

@pzuraq
Copy link
Contributor

pzuraq commented Mar 14, 2022

This PR is good as long as it's followed shortly with the changes in #14353

@JLHwung JLHwung merged commit b636306 into babel:main Mar 14, 2022
@JLHwung JLHwung deleted the decorator-misc-fix branch March 14, 2022 17:54
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jun 14, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 14, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated A closed issue/PR that is archived due to age. Recommended to make a new issue PR: Spec Compliance 👓 A type of pull request used for our changelog categories Spec: Decorators
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants