environmentVisitor should skip decorator expressions #14371
Conversation
JLHwung
added
the
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51986/ |
| @@ -47,4 +46,4 @@ export default { | |||
| ClassProperty(path) { | |||
| skipAndrequeueComputedKeysAndDecorators(path); | |||
| }, | |||
| } as Visitor<PluginPass>; | |||
| } as Visitor<any>; | |||
There was a problem hiding this comment.
Does Visitor<unknown> work here? If it does, it's safer since it doesn't "disable" type checking.
| const keys = VISITOR_KEYS[path.type]; | ||
| for (const key of keys) { | ||
| if (key !== "key") path.skipKey(key); | ||
| function skipAndrequeueComputedKeysAndDecorators( |
There was a problem hiding this comment.
| function skipAndrequeueComputedKeysAndDecorators( | |
| function skipAndRequeueComputedKeysAndDecorators( |
(or maybe something with less words like skipInternalContext or skipContents works too).
There was a problem hiding this comment.
I think skipInternalContext and skipContents are a bit vague since they are not formally defined in the spec. Maybe skipAndRequeueKeyAndDecorator? I also thought about skipParamAndBodyAndInitializer but it seems no better than skipAndRequeueKeyAndDecorator.
| const keys = VISITOR_KEYS[path.type]; | ||
| for (const key of keys) { | ||
| if (key !== "key") path.skipKey(key); | ||
| function skipAndrequeueComputedKeysAndDecorators(path: NodePath) { |
There was a problem hiding this comment.
Can we import the logic from the helper?
path.isMethod() ? skipAndRequeueComputedKeysAndDecorators(path) : path.skip();
JLHwung
force-pushed
the
environment-visitor
branch
from
5717af3
to
b35e893
Compare
| const keys = VISITOR_KEYS[path.type]; | ||
| for (const key of keys) { | ||
| if (key !== "key") path.skipKey(key); | ||
| export function requeueComputedKeyAndDecorator( |
There was a problem hiding this comment.
| export function requeueComputedKeyAndDecorator( | |
| export function requeueComputedKeyAndDecorators( |
I like how the new behavior gave us a more readable name!
JLHwung
force-pushed
the
environment-visitor
branch
from
8f8ccae
to
34fb84c
Compare
|
@JLHwung It has been two months since your last update to this PR, time for a self-review? 🙏 |
JLHwung
force-pushed
the
environment-visitor
branch
from
34fb84c
to
791d2f4
Compare
| "ClassProperty|ClassPrivateProperty"( | ||
| path: NodePath<t.ClassProperty | t.ClassPrivateProperty>, | ||
| ) { | ||
| Property(path) { |
There was a problem hiding this comment.
The visitor now hooks on Property alias so that it supports older @babel/types without ClassAccessorProperty definitions.
| @@ -16,7 +16,7 @@ let Outer = /*#__PURE__*/function (_Hello) { | |||
| var _this; | |||
|
|
|||
| babelHelpers.classCallCheck(this, Outer); | |||
| _dec = _this = _super.call(this) | |||
| _dec = _this = _super.call(this); | |||
There was a problem hiding this comment.
I don't know why this line is changed after rebasing. Maybe a generator update?
| @@ -511,7 +511,7 @@ function transformClass( | |||
| const newField = generateClassProperty(newId, valueNode, isStatic); | |||
|
|
|||
| const [newPath] = element.replaceWith(newField); | |||
| addProxyAccessorsFor(newPath, key, newId, element.node.computed); | |||
| addProxyAccessorsFor(newPath, key, newId, computed); | |||
There was a problem hiding this comment.
This is a bug fix, the element.node.computed is of the replacement node, which is always false.
github-actions
bot
added
the
outdated
Fixes #1, Fixes #2In this PR we reimplement
skipAllButComputedKeyso we can remove@babel/typesdeps of the environment-visitor helper. In the 2nd commit we expand similar logic to decorator expressions as well. Note that the test cases are already passing in main because the decorator transforms will hoist the expression. In the last commit we apply similar changes to the renamer.