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

fix: Decorators when super() exists and protoInit is not needed #16385

Merged
merged 3 commits into from Mar 27, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -475,26 +475,28 @@ function optimizeSuperCallAndExpressions(
expressions: t.Expression[],
protoInitLocal: t.Identifier,
) {
// Merge `super(), protoInit(this)` into `protoInit(super())`
if (
expressions.length >= 2 &&
isProtoInitCallExpression(expressions[1], protoInitLocal)
) {
const mergedSuperCall = t.callExpression(t.cloneNode(protoInitLocal), [
expressions[0],
]);
expressions.splice(0, 2, mergedSuperCall);
}
// Merge `protoInit(super()), this` into `protoInit(super())`
if (
expressions.length >= 2 &&
t.isThisExpression(expressions[expressions.length - 1]) &&
isProtoInitCallExpression(
expressions[expressions.length - 2],
protoInitLocal,
)
) {
expressions.splice(expressions.length - 1, 1);
if (protoInitLocal) {
if (
expressions.length >= 2 &&
isProtoInitCallExpression(expressions[1], protoInitLocal)
) {
// Merge `super(), protoInit(this)` into `protoInit(super())`
const mergedSuperCall = t.callExpression(t.cloneNode(protoInitLocal), [
expressions[0],
]);
expressions.splice(0, 2, mergedSuperCall);
}
// Merge `protoInit(super()), this` into `protoInit(super())`
if (
expressions.length >= 2 &&
t.isThisExpression(expressions[expressions.length - 1]) &&
isProtoInitCallExpression(
expressions[expressions.length - 2],
protoInitLocal,
)
) {
expressions.splice(expressions.length - 1, 1);
}
}
return maybeSequenceExpression(expressions);
}
Expand Down
@@ -0,0 +1,24 @@
function decorate() {
return function (target, context) {}
}

class Test {
@decorate()
accessor foo = 42;

constructor() {
}
}

new Test()

class TestChild extends Test {
@decorate()
accessor bar = 1;

constructor() {
super();
}
}

const r = new TestChild();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In contrast with initProto-existing-derived-constructor, can you rename this test to initField-existing-derived-constructor?

I suggest we also add some behaviour assertions like we have done in that test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried porting all the tests but unfortunately couldn't get them all to work so I just added a simple test.🤦‍♂️

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you push the new tests? Even if they are failing, we can still investigate for sure.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
I pushed it, but it doesn't show up on github, I will try force pushing.🤔

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-added all the tests, but I'm not sure if I tested the behavior well, thanks for the review!

@@ -0,0 +1,25 @@
function decorate() {
return function (target, context) {}
}

class Test {
@decorate()
accessor foo = 42;

constructor() {
console.log('hello');
}
}

new Test()

class TestChild extends Test {
@decorate()
accessor bar = 1;

constructor() {
super();
}
}

const r = new TestChild();
@@ -0,0 +1,37 @@
var _Test, _TestChild;
let _init_foo, _init_extra_foo, _init_bar, _init_extra_bar;
function decorate() {
return function (target, context) {};
}
var _A = /*#__PURE__*/new WeakMap();
class Test {
get foo() {
return babelHelpers.classPrivateFieldGet2(_A, this);
}
set foo(v) {
babelHelpers.classPrivateFieldSet2(_A, this, v);
}
constructor() {
babelHelpers.classPrivateFieldInitSpec(this, _A, _init_foo(this, 42));
_init_extra_foo(this);
console.log('hello');
}
}
_Test = Test;
[_init_foo, _init_extra_foo] = babelHelpers.applyDecs2311(_Test, [], [[decorate(), 1, "foo"]]).e;
new Test();
var _A2 = /*#__PURE__*/new WeakMap();
class TestChild extends Test {
get bar() {
return babelHelpers.classPrivateFieldGet2(_A2, this);
}
set bar(v) {
babelHelpers.classPrivateFieldSet2(_A2, this, v);
}
constructor() {
(super(), babelHelpers.classPrivateFieldInitSpec(this, _A2, _init_bar(this, 1)), this), _init_extra_bar(this);
}
}
_TestChild = TestChild;
[_init_bar, _init_extra_bar] = babelHelpers.applyDecs2311(_TestChild, [], [[decorate(), 1, "bar"]], 0, void 0, Test).e;
const r = new TestChild();
@@ -0,0 +1,24 @@
function decorate() {
return function (target, context) {}
}

class Test {
@decorate()
accessor foo = 42;

constructor() {
}
}

new Test()

class TestChild extends Test {
@decorate()
accessor bar = 1;

constructor() {
super();
}
}

const r = new TestChild();
@@ -0,0 +1,25 @@
function decorate() {
return function (target, context) {}
}

class Test {
@decorate()
accessor foo = 42;

constructor() {
console.log('hello');
}
}

new Test()

class TestChild extends Test {
@decorate()
accessor bar = 1;

constructor() {
super();
}
}

const r = new TestChild();
@@ -0,0 +1,37 @@
let _init_foo, _init_extra_foo, _init_bar, _init_extra_bar;
function decorate() {
return function (target, context) {};
}
class Test {
static {
[_init_foo, _init_extra_foo] = babelHelpers.applyDecs2311(this, [], [[decorate(), 1, "foo"]]).e;
}
#A = _init_foo(this, 42);
get foo() {
return this.#A;
}
set foo(v) {
this.#A = v;
}
constructor() {
_init_extra_foo(this);
console.log('hello');
}
}
new Test();
class TestChild extends Test {
static {
[_init_bar, _init_extra_bar] = babelHelpers.applyDecs2311(this, [], [[decorate(), 1, "bar"]], 0, void 0, Test).e;
}
#A = _init_bar(this, 1);
get bar() {
return this.#A;
}
set bar(v) {
this.#A = v;
}
constructor() {
super(), _init_extra_bar(this);
}
}
const r = new TestChild();