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/resolver): Resolve top-level undefined, NaN, and Infinity correctly #8471

Merged
merged 2 commits into from
Jan 4, 2024

Conversation

Austaras
Copy link
Member

@Austaras Austaras commented Jan 2, 2024

…aration correctly in es module

Description:

For following code

var NaN = 1
console.log(NaN)

Result would be

Envirnoment Result
Non strict script(browser, nodejs repl) NaN
Non strict script(nodejs script) 1
Strict script(browser, nodejs repl) runtime error
Strict script(nodejs script) 1
ESM 1

So SWC choose to behave like browser in script mode and confirm to esm standard.

BREAKING CHANGE:

Related issue (if exists):
Closes #8465

@kdy1
Copy link
Member

kdy1 commented Jan 2, 2024

I think the problem here may be the fact that we use ESM mode for terser test suites

@Austaras
Copy link
Member Author

Austaras commented Jan 2, 2024

I think the problem here may be the fact that we use ESM mode for terser test suites

Yes. I would open a separate PR for this. Doesn't seem like to have any problem.

@Austaras
Copy link
Member Author

Austaras commented Jan 2, 2024

Some investigation, for following code

var NaN = 1
console.log(NaN)

Result would be

Envirnoment Result
Non strict script(browser, nodejs repl) NaN
Non strict script(nodejs script) 1
Strict script(browser, ndoejs repl) runtime error
Strict script(nodejs script) 1
ESM 1

Seems like the benefit of this mechanism is very limited. Maybe just remove it?

@kdy1
Copy link
Member

kdy1 commented Jan 2, 2024

I agree that the benefit is small, but I also think code is trivial enough to justify the size of benefit

@kdy1 kdy1 added this to the Planned milestone Jan 2, 2024
@kdy1 kdy1 changed the title fix(es/resolver): Resolve top level undefined NaN Infinity decl… fix(es/resolver): Resolve top-level undefined, NaN, and Infinity correctly Jan 2, 2024
@Austaras
Copy link
Member Author

Austaras commented Jan 2, 2024

Fine. Let's proceed this way.

kdy1
kdy1 previously approved these changes Jan 2, 2024
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 all!


swc-bump:

  • swc_ecma_ast
  • swc_ecma_transforms_base

@kdy1 kdy1 enabled auto-merge (squash) January 2, 2024 05:06
@Austaras Austaras disabled auto-merge January 2, 2024 05:17
@magic-akari
Copy link
Member

Maybe just remove it?

What does this mean? Should SWC skip them when renaming?

@Austaras
Copy link
Member Author

Austaras commented Jan 2, 2024

Maybe just remove it?

What does this mean? Should SWC skip them when renaming?

Maybe just remove the special treatment for undefined etc like what babel do. But that's not very good for minifier

@Austaras Austaras requested a review from kdy1 January 4, 2024 01:24
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) January 4, 2024 03:33
@kdy1 kdy1 added the automerge label Jan 4, 2024
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 82bd807 into swc-project:main Jan 4, 2024
276 of 277 checks passed
@kdy1 kdy1 modified the milestones: Planned, v1.3.103 Jan 15, 2024
@swc-project swc-project locked as resolved and limited conversation to collaborators Feb 14, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

wrongly transform function Infinity to 1/0
4 participants