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/codegen): Fix placing of comments of yield arguments #7858

Merged
merged 3 commits into from
Aug 27, 2023

Conversation

lewisl9029
Copy link
Contributor

@lewisl9029 lewisl9029 commented Aug 25, 2023

Description:

Looks like the bug I ran into had nothing to do with the changes in #7856, since it's reproducible without it. Looks like it might have only surfaced now because #7853 changed the default value of jsc.minify.format.comments? Added a minimal test case here with the expected result.

Here's the actual output:

 export var padding = '';
 export function exec2({ commands }) {
     return __awaiter(this, void 0, void 0, function*() {
         for(let i2 = 0; i2 < commands.length; i2++){
             let command = commands[i2];
             yield // some-comment
             function({ command }) {
                 command();
             }({
                 command,
                 handleError
             });
         }
     });
 }

The comment ends up getting added after the yield, which makes the output invalid.

Going to see if I can figure out a fix tomorrow, but let me know if you have any ideas on where to start looking in the meantime!

@kdy1
Copy link
Member

kdy1 commented Aug 25, 2023

We may need need_paren like in

let need_paren = n
.arg
.as_deref()
.map(|expr| self.has_leading_comment(expr))
.unwrap_or(false);
for emit_yield_expr too.

if !node.delegate && arg.starts_with_alpha_num() {
space!()
} else {
formatting_space!()
}

@lewisl9029 lewisl9029 force-pushed the misplaced-comment-after-yield branch from 4630e76 to 7d1bc9f Compare August 25, 2023 20:35
@lewisl9029 lewisl9029 marked this pull request as ready for review August 25, 2023 20:36
@lewisl9029
Copy link
Contributor Author

@kdy1 thank you so much for the suggestion, it worked great!
Added the fix and updated the expected output accordingly in 7d1bc9f

@kdy1 kdy1 changed the title Misplaced comment after minification fix(es/codegen): Fix placing of comments of yield arguments Aug 27, 2023
@kdy1 kdy1 added this to the Planned milestone Aug 27, 2023
Copy link
Member

@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.

Thank you!


swc-bump:

  • swc_ecma_codegen

@kdy1 kdy1 enabled auto-merge (squash) August 27, 2023 22:55
Copy link
Collaborator

@swc-bot swc-bot left a 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


@kdy1 kdy1 merged commit 122d14c into swc-project:main Aug 27, 2023
@kdy1 kdy1 modified the milestones: Planned, v1.3.81 Aug 30, 2023
kdy1 added a commit that referenced this pull request Sep 25, 2023
**Description:**


x-ref: https://vercel.slack.com/archives/C02HY34AKME/p1695334071194139


Reproduction: https://github.com/kdy1/repro-test-mdx-korean



Regression of next.js: `v13.4.11`(swc_core@v0.79.13) => `v13.5.2`
(swc_core@v0.83.12)


 - `next@v13.4.19` works (swc_core@v0.79.59)
 - `next@v13.4.20-canary.3` works (swc_core@v0.79.70)
 -  **`next@v13.4.20-canary.32` fails** (swc_core@v0.83.12)

Commit range:
662f236...e67bf05

- `swc_core@v0.79.70`:
662f236
- `swc_core@v0.83.12`:
e67bf05

Diff: https://gist.github.com/kdy1/047e7e5537c34180d446cb3d5b95fce8

---

I did more investigation by monkey-patching the `next` package.
`.minify()` resolved without an exception.
It means that `.minify()` is producing an invalid ES code.

So... candidates are

 - #7890
 - #7876
 - #7858
 - #7856
 - #7853
 - #7832
@swc-project swc-project locked as resolved and limited conversation to collaborators Sep 29, 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.

None yet

3 participants