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): Use unsafe option for arrow => method #6521

Merged
merged 15 commits into from Nov 29, 2022

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Nov 28, 2022

Description:

Related issue:


Investigation

This is caused by arrows, and seems like a same issue as #6507

({foo(){}}).foo.toString()

prints foo(){}.

@kdy1 kdy1 added this to the Planned milestone Nov 28, 2022
@kdy1 kdy1 self-assigned this Nov 28, 2022
@kdy1 kdy1 changed the title fix(es/minifier): Fix arrows fix(es/minifier): Disable arrows if eval exists Nov 28, 2022
@kdy1 kdy1 changed the title fix(es/minifier): Disable arrows if eval exists fix(es/minifier): Use unsafe option for arrow => method Nov 28, 2022
@kdy1 kdy1 requested a review from jridgewell November 28, 2022 04:33
@kdy1 kdy1 marked this pull request as ready for review November 28, 2022 04:33
kodiakhq[bot]
kodiakhq bot previously approved these changes Nov 28, 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_minifier

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Does this really fix either bug? It seems like we're just avoiding it because we're not converting to method shorthand anymore, but the real bug is that the compresser is omitting the {} enclosing object?

@kdy1
Copy link
Member Author

kdy1 commented Nov 29, 2022

@jridgewell

{} not omitted by the compressor/codegen.
The problem is eval and .toString() of arrow functions.

({foo(){}}).foo.toString() prints 'foo(){}', and if it's used in eval, the problem occurs

Copy link
Contributor

@jridgewell jridgewell left a comment

Choose a reason for hiding this comment

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

Ok, crawling through the bugs it's being caused because they're doing string concatenation of functions, which is inherently buggy if we're minifying it.

@kdy1 kdy1 enabled auto-merge (squash) November 29, 2022 03:51
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 9752b43 into swc-project:main Nov 29, 2022
@kdy1 kdy1 deleted the next-43208 branch November 29, 2022 05:23
@kdy1 kdy1 modified the milestones: Planned, v1.3.21 Nov 30, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Dec 29, 2022
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.

swc minifier breaks hls.js
3 participants