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/minifier): Don't remove exports #7856

Merged
merged 3 commits into from
Aug 25, 2023

Conversation

lewisl9029
Copy link
Contributor

Description:

Hi there! I recently upgraded SWC from 1.3.60 to the latest 1.3.78 and discovered that some of my builds were broken because SWC's minifier was removing exports.

I've added some minimal repros of the cases I encountered as failing test cases, and a small change to the minifier to fix them.

Happy to iterate on this as you see fit. Cheers!

@CLAassistant
Copy link

CLAassistant commented Aug 24, 2023

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@lewisl9029 lewisl9029 force-pushed the fix-minifier-removing-exports branch from b890409 to 70d8586 Compare August 24, 2023 23:45
@lewisl9029 lewisl9029 marked this pull request as ready for review August 24, 2023 23:46
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_minifier

@lewisl9029
Copy link
Contributor Author

Oops, this seems to be causing some new issues around comments being moved to the wrong places...

Working on a test case and fix.

@kdy1 kdy1 changed the title Fix minifier removing exports fix(es/minifier): Don't remove exporrs Aug 25, 2023
@kdy1 kdy1 changed the title fix(es/minifier): Don't remove exporrs fix(es/minifier): Don't remove exports Aug 25, 2023
@kdy1 kdy1 self-assigned this Aug 25, 2023
@kdy1 kdy1 added this to the Planned milestone Aug 25, 2023
@kdy1 kdy1 enabled auto-merge (squash) August 25, 2023 02:55
@lewisl9029
Copy link
Contributor Author

@kdy1 looks like we commented at the same time haha...

Closing for now to make sure this doesn't get merged as is since it's causing some issues in my local testing. Working on an update, will reopen once it's ready for another look!

@lewisl9029 lewisl9029 closed this Aug 25, 2023
auto-merge was automatically disabled August 25, 2023 02:59

Pull request was closed

@kdy1
Copy link
Member

kdy1 commented Aug 25, 2023

Well, you don't need to worry about it. SWC has a good CI process, and once CI issue is fixed by #7857 the @swc-bot will rebase this

@kdy1 kdy1 reopened this Aug 25, 2023
@kdy1 kdy1 enabled auto-merge (squash) August 25, 2023 03:01
@lewisl9029
Copy link
Contributor Author

Got it, will keep looking into the issue from my side, and send in another PR if needed! :)

@kdy1
Copy link
Member

kdy1 commented Aug 25, 2023

스크린샷 2023-08-25 오후 12 02 00

image

@kdy1
Copy link
Member

kdy1 commented Aug 25, 2023

@lewisl9029 Please enable Allow edit by maintainers

@lewisl9029
Copy link
Contributor Author

Hey @kdy1, didn't realize this before, but it looks like GitHub doesn't actually support enabling that option for forks in organizations 🤦

If the edits you're looking to make are small, just let me know what they are and I'd be happy to go and make them. Alternatively, feel free to just cherry pick the changes here onto your own branch if that's easier. I'll remember to make a fork under my personal account the next time I need to contribute. Thanks!

@kdy1
Copy link
Member

kdy1 commented Aug 25, 2023

Then can you rebase this PR?

@kdy1
Copy link
Member

kdy1 commented Aug 25, 2023

Pressing Update branch button is fine

@lewisl9029
Copy link
Contributor Author

Done!

@kdy1 kdy1 merged commit ae8cd94 into swc-project:main Aug 25, 2023
250 checks passed
@kdy1 kdy1 modified the milestones: Planned, v1.3.79 Aug 25, 2023
@lewisl9029 lewisl9029 deleted the fix-minifier-removing-exports branch August 25, 2023 04:54
kdy1 pushed a commit that referenced this pull request Aug 27, 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:

```js
 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!
@swc-project swc-project locked as resolved and limited conversation to collaborators Sep 24, 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