-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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(es/compat): Fix handling of private members in optional chaining pass #7610
Conversation
c8bcf45
to
dcef8e6
Compare
dcef8e6
to
c4ac964
Compare
test() { | ||
(_this = this) === null || _this === void 0 | ||
? void 0 | ||
: _class_private_field_get(_this.y , _x); |
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.
This is wrong.
class Foo {
#x;
test() {
this === null || this === void 0 ? void 0 : this.y.#x;
}
}
Please try locally using
{
"plugins": [
"@babel/plugin-transform-optional-chaining"
]
}
This is how I got the expected output
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.
Hey @kdy1,
On the original issue: #7561, @jridgewell says that this is the expected output:
// …
var _x = new WeakMap();
class Foo {
test() {
(_this = this) === null || _this === void 0
? void 0
: _class_private_field_get(_this.y , _x);
}
// …
}
Also, isn't your output the same as mine, but without the transformation for the private field?
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.
PS: I thought that the main problem was that we were not transforming the private field.
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.
No. Expected output of optional-chaining
is mine. What you uploaded is output of full transforms.
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.
I'm sorry, @kdy1, I don't think I understand your comment.
Here is the problem that I want to solve with this PR (please note that I haven't been able to actually solve it yet, this is why you only saw a test): whenever we have an optional chaining expression with a private field, the private field is not transformed (we don't replace it with: _class_private_field_get
).
This is what I understood from these issues: #7561 and #7559.
Regarding your comment:
No. Expected output of optional-chaining is mine. What you uploaded is output of full transforms.
Do you mean that I've put the test in the wrong place? Since it is inside the optional chaining module, it shouldn't be testing private fields? If so, where would be the most appropriate place to put it?
Or do you mean that I am solving the wrong problem?
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.
whenever we have an optional chaining expression with a private field, the private field is not transformed
This is wrong. Private fields are transformed by another pass.
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.
The optional chaining pass should produce
class Foo {
#x;
test() {
this === null || this === void 0 ? void 0 : this.y.#x;
}
}
and another pass is responsible for the rest.
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.
Ok, there are two problems?
One for the optional chaining and another for tge private field?
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.
No, private field transforms are working correctly
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.
I've updated the test as you requested. I think it is correct now.
796605b
to
1b18e8a
Compare
1b18e8a
to
aded348
Compare
Hey @kdy1, I believe I have solved the problem. Could you review it please? |
@@ -303,3 +303,16 @@ fn fixture_loose(input: PathBuf) { | |||
Default::default(), | |||
); | |||
} | |||
|
|||
#[testing::fixture("tests/optional-chaining-private-var/issue-7561/input.js")] |
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.
Please remove this
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.
You don't need to add a new function with testing::fixture
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.
Just add a test file to the correct directory.
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.
I think I've added it to the correct folder, now. Could you check, please?
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.
Thank you, nice work!
swc-bump:
- swc_ecma_transforms_compat
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.
Automated review comment generated by auto-rebase script
…haining pass (swc-project#7610)" This reverts commit 7ba7b6e.
I'm reverting this commit with #7812 |
…haining pass (swc-project#7610)" This reverts commit 7ba7b6e.
…haining pass (swc-project#7610)" This reverts commit 7ba7b6e.
Description:
Fix: #7561
Related issue (if exists):
#7561