Fix 2021-12 decorators application order #14244
Conversation
JLHwung
added
PR: Bug Fix 🐛
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51192/ |
packages/babel-plugin-proposal-decorators/src/transformer-2021-12.ts
Outdated
Show resolved
Hide resolved
| @@ -879,6 +875,7 @@ function transformClass( | |||
| const originalClass = path.node; | |||
|
|
|||
| if (classDecorators) { | |||
| locals.push(classLocal, classInitLocal); | |||
There was a problem hiding this comment.
classLocal and classInitLocal are generated after other functions, see applyClassDecs in applyDecs.
| @@ -317,7 +317,7 @@ function applyMemberDec( | |||
| } | |||
| } | |||
| } else { | |||
| for (var i = 0; i < decs.length; i++) { | |||
| for (var i = decs.length - 1; i >= 0; i--) { | |||
There was a problem hiding this comment.
Decorators (after evaluated) are applied from right to left.
| @@ -13,7 +13,7 @@ function dec2(_, { setMetadata, getMetadata }) { | |||
| } | |||
|
|
|||
| class Foo { | |||
| @dec1 @dec2 a; | |||
| @dec2 @dec1 a; | |||
There was a problem hiding this comment.
In this test we check whether desc2 should see the metadata changed by desc1. Because decorators are applied from right to left, the order should be swapped.
| @@ -0,0 +1,31 @@ | |||
| var log = []; | |||
There was a problem hiding this comment.
This test is revised from https://github.com/babel/babel/blob/89e26a0528cf557bac53c28b1a5918c2f9089681/packages/babel-plugin-proposal-decorators/test/fixtures/ordering/decorators/exec.js
It ensures that #14242 is fixed. I plan to extend this test and cover more decorator features as well as computed property keys.
JLHwung
force-pushed
the
fix-apply-decs-ordering-14242
branch
from
6e7dec0
to
c1fe11d
Compare
There was a problem hiding this comment.
@pzuraq We usually ask for two ✔️ from Babel team members, but in this case a review from you would be more valuable than one from us 🙏
This reverts commit 4ef1699. # Conflicts: # packages/babel-helpers/src/helpers-generated.ts
JLHwung
force-pushed
the
fix-apply-decs-ordering-14242
branch
from
c4924c0
to
ca71772
Compare
| } | ||
| } | ||
| } | ||
| expect(receivedName).toBe("B"); |
There was a problem hiding this comment.
Added a new test on decorator evaluation scope. I didn't copy the test to es2015 because it is currently failing: The staticBlockToIIFE routine losts the class environment and this no more points to class B.
Update: the new test is passing in es2015. But when the class is nested in static block, es2015 test will fail.
|
|
||
| class Foo { | ||
| static { | ||
| [_initProto] = babelHelpers.applyDecs(this, [[_dec, 2, "a"], [_dec2, 2, _computedKey]], []); | ||
| [_initProto] = babelHelpers.applyDecs(this, [[dec, 2, "a"], [dec, 2, _computedKey]], []); |
There was a problem hiding this comment.
The new memoising approach generates less code when dec is considered static, e.g. a bound identifier.
There was a problem hiding this comment.
Oh good, this will probably be a common case.
EDIT: It won't be common because most decorators will be imported, nevermind.
|
|
||
| return class Nested { | ||
| return _dec9 = this.#a, class Nested { |
There was a problem hiding this comment.
The evaluation scope issue is also captured in this test. this.#a should be evaluated under makeClass.
|
The failing test needs a |
JLHwung
force-pushed
the
fix-apply-decs-ordering-14242
branch
from
a5dc51e
to
796ccba
Compare
nicolo-ribaudo
changed the title
github-actions
bot
added
the
outdated
Also fixed another two bugs. See explainers below.
/cc @pzuraq (I can't request your review because of org issues)