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): Abort IIFE invoker on eval #6478

Merged
merged 64 commits into from Dec 13, 2022
Merged

fix(es/minifier): Abort IIFE invoker on eval #6478

merged 64 commits into from Dec 13, 2022

Conversation

kdy1
Copy link
Member

@kdy1 kdy1 commented Nov 18, 2022

Description:

Related issue:


Investigation

This is an issue of invoke_iife. I used patch script to check each function.

image

There are two variables named exports

@kdy1 kdy1 added this to the Planned milestone Nov 18, 2022
@kdy1 kdy1 self-assigned this Nov 18, 2022
@kdy1 kdy1 changed the title fix(es/minifier): Fix a bug of inliner fix(es/minifier): Fix variable remapping of IIFE invoker Dec 8, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 8, 2022
crates/swc_ecma_minifier/tests/fixture/next/43052/input.js Outdated Show resolved Hide resolved
@@ -728,6 +728,13 @@ where
}
}

// If we has an eval, we cannot remap variables correctly.
let has_eval = contains_eval(body, false);
if !param_ids.is_empty() && has_eval {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm kinda surprised we allow any further modifications in the presence of eval.

Copy link
Member Author

@kdy1 kdy1 Dec 9, 2022

Choose a reason for hiding this comment

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

eval is used by some libraries, so I thought we should only give up if it's going to be problematic. (e.g. name mangler aborts)

Copy link
Member Author

Choose a reason for hiding this comment

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

But I'm not sure about this. Do you think it's better to abort completely on eval?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally, yah, I would. Looking at terser, eval deopts:

  • name mangling
  • unused var removal
  • joining of expressions
  • inlining functions
  • conversion from functions/methods to arrows

Copy link
Member Author

Choose a reason for hiding this comment

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

Did it and checked other passes, too. Thank you!

kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 9, 2022
kodiakhq[bot]
kodiakhq bot previously approved these changes Dec 9, 2022
@kdy1 kdy1 requested a review from jridgewell December 9, 2022 05:30
@kdy1 kdy1 enabled auto-merge (squash) December 13, 2022 03:33
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 8b2e1d1 into swc-project:main Dec 13, 2022
@kdy1 kdy1 deleted the bug1 branch December 13, 2022 04:18
@kdy1 kdy1 modified the milestones: Planned, v1.3.23 Dec 14, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Jan 13, 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