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 track usage assign #6044

Merged
merged 1 commit into from
Oct 6, 2022
Merged

Conversation

Austaras
Copy link
Member

@Austaras Austaras commented Oct 5, 2022

Description:
Don't know if it's right, but it generate correct output.

BREAKING CHANGE:

Related issue (if exists):

@kdy1 kdy1 added this to the Planned milestone Oct 5, 2022
@kdy1 kdy1 self-assigned this Oct 5, 2022
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

@Austaras
Copy link
Member Author

Austaras commented Oct 5, 2022

Oh, sorry

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.

I'm not sure if we are proving these facts.
I added two-way infection, which is clearly not optimal, because it prevents bugs

@Austaras
Copy link
Member Author

Austaras commented Oct 5, 2022

Yeah, if isObject is a normal function it would work fine. Maybe there's some more complex mechanism.

@Austaras
Copy link
Member Author

Austaras commented Oct 5, 2022

The difference can be run down to https://github.com/terser/terser/blob/master/lib/compress/tighten-body.js#L887. It checks if it's a AST_SymbolDefun(definition of function) or defined in current scope, which I believe is just a convenient implementation.

@kdy1
Copy link
Member

kdy1 commented Oct 5, 2022

Oh... It's bit shocking but if so I think this PR is fine

@Austaras
Copy link
Member Author

Austaras commented Oct 5, 2022

The difference can be run down to https://github.com/terser/terser/blob/master/lib/compress/tighten-body.js#L887. It checks if it's a AST_SymbolDefun(definition of function) or defined in current scope, which I believe is just a convenient implementation.

So basically function isObject is fine, but const isObject = function is fine only if it's defined in same scope of newValue.

jridgewell
jridgewell previously approved these changes Oct 5, 2022
kdy1
kdy1 previously approved these changes Oct 6, 2022
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.

This PR seems innocent.
I'll merge this after merging #6053


swc-bump:

  • swc_ecma_minifier

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!

@kdy1 kdy1 enabled auto-merge (squash) October 6, 2022 07:21
@kdy1 kdy1 disabled auto-merge October 6, 2022 07:33
@kdy1 kdy1 enabled auto-merge (squash) October 6, 2022 07:48
@kdy1 kdy1 disabled auto-merge October 6, 2022 08:30
@kdy1 kdy1 merged commit c9427f1 into swc-project:main Oct 6, 2022
@kdy1 kdy1 modified the milestones: Planned, v1.3.6 Oct 8, 2022
@swc-project swc-project locked as resolved and limited conversation to collaborators Nov 7, 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.

3 participants