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: ASI in concatenated module only when necessary #18709

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

fi3ework
Copy link
Contributor

What kind of change does this PR introduce?

Only add a prefix semicolon in the concatenated module when necessary.

The fixed semicolon was introduced in #11897. The strange part is that the test case in that pull request does not work because it requires enabling ⁠optimization.concatenateModules. Therefore, I moved the case to the config cases. The test cases will fail when I only remove the semicolon ⁠in \n;// CONCATENATED MODULE: without making any other changes, indicating that it is now functioning normally.

Related tracking issue web-infra-dev/rspack#7591 (comment).

Did you add tests for your changes?

Yes.

Does this PR introduce a breaking change?

No.

What needs to be documented once your changes are merged?

No.

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

@alexander-akait alexander-akait merged commit 94aba38 into webpack:main Aug 27, 2024
57 checks passed
@alexander-akait
Copy link
Member

alexander-akait commented Aug 27, 2024

I hope we covered all possible cases 😄 maybe make sense to add more tests

@fi3ework
Copy link
Contributor Author

fi3ework commented Aug 27, 2024

The current implementation is somewhat saturated. Semicolons will be added in two situations:

  1. If the code generated by each module's code generation starts with CHARS_REQUIRING_SEMICOLON, a semicolon will be added directly at the connection point.
  2. When replacing finalName, if it starts with CHARS_REQUIRING_SEMICOLON, a semicolon must be added at the concatenation point. Although this semicolon is not always necessary, it is difficult to determine when finalName will appear at the beginning of the module, so it is added as a precaution.

In other cases, ASI should be evaluated in JavascriptParser. Here, we only assess scenarios where concatenating two modules requires ASI. CHARS_REQUIRING_SEMICOLON currently refers to situations where semicolons must be added at the beginning of a statement in JavaScript. Do you have any other suggestions for the cases that not covered? Thank you!

@alexander-akait
Copy link
Member

@fi3ework
Copy link
Contributor Author

Got it, I'll update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants