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(es/fixer): Preserve parens of optional chaining expressions #6164

Closed
wants to merge 16 commits into from

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Oct 15, 2022

@kdy1 kdy1 added this to the Planned milestone Oct 15, 2022
@kdy1 kdy1 self-assigned this Oct 15, 2022
@kdy1 kdy1 changed the title Fix fix(es/fixer): Wrap opt chain base of a member expression Oct 15, 2022
@kdy1 kdy1 marked this pull request as ready for review October 15, 2022 07:02
@kdy1 kdy1 enabled auto-merge (squash) October 15, 2022 07:02
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 15, 2022
Copy link
Member Author

@kdy1 kdy1 left a comment

Choose a reason for hiding this comment

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

swc-bump:

  • swc_ecma_transforms_base

kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 15, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 15, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 15, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Oct 15, 2022
@kdy1 kdy1 marked this pull request as draft October 15, 2022 08:54
auto-merge was automatically disabled October 15, 2022 08:54

Pull request was converted to draft

@kdy1 kdy1 changed the title fix(es/fixer): Wrap opt chain base of a member expression fix(es/fixer): Preserve parens of optional chaining expressions Oct 21, 2022
@@ -577,6 +577,18 @@ impl VisitMut for Fixer<'_> {

_ => {}
}

dbg!(&*n.obj);
Copy link
Member Author

Choose a reason for hiding this comment

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

cc @Austaras I think AST definition is wrong.
We can't store ParenExpr in OptChainBase. Any idea?
Should we add a variant to it?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it should be Paren (OptChain (OptChainMember))?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does not work.

global.a = null;
let foo = "PASS";
a?.b[(foo = "FAIL")];
console.log(foo);

this is a terser test case, and it should not be

global.a = null;
let foo = "PASS";
(a?.b)[(foo = "FAIL")];
console.log(foo);

Copy link
Member

Choose a reason for hiding this comment

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

If a?.b[(foo = "FAIL")]; is the raw input, it should be OptChain(OptChainMember (Member)) while (a?.b)[(foo = "FAIL")] should be Member(OptChain(OptChainMember)), at least this is what babel is doing

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! I'm not sure if it's ideal, but we can avoid a breaking change for wasm plugin authors with that way 👍

@@ -242,7 +242,7 @@ let NATIVE_OS = "linux";
const navigator = globalThis.navigator;
if (globalThis.Deno != null) {
NATIVE_OS = Deno.build.os;
} else if (navigator?.appVersion?.includes?.("Win") ?? false) {
} else if ((navigator?.appVersion)?.includes?.("Win") ?? false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

In this very specific case (foo?.bar)?.baz, you can't actually observe the difference, because the chain is immediately made optional again.

@kdy1 kdy1 closed this Jan 13, 2023
@kdy1 kdy1 deleted the fix branch January 13, 2023 06:50
@kdy1 kdy1 modified the milestones: Planned, v1.3.27 Jan 17, 2023
@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Minifier removes parens around optional chain followed by member access
3 participants