Skip to content

fix(external_module): use "const" if supported when concatenation#7836

Merged
fi3ework merged 1 commit intoweb-infra-dev:mainfrom
fi3ework:const-con-exter
Sep 10, 2024
Merged

fix(external_module): use "const" if supported when concatenation#7836
fi3ework merged 1 commit intoweb-infra-dev:mainfrom
fi3ework:const-con-exter

Conversation

@fi3ework
Copy link
Copy Markdown
Member

@fi3ework fi3ework commented Sep 9, 2024

Summary

Align with https://github.com/webpack/webpack/blob/039e8c96338ee98cf37d5035cdd30073a96b4ec5/lib/ExternalModule.js#L878. No coveraged test found in webpack.

Checklist

  • Tests updated (or not required).
  • Documentation updated (or not required).

@netlify
Copy link
Copy Markdown

netlify bot commented Sep 9, 2024

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit e2e3cc1
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/66dec6fd9253c50008383cd0

@netlify
Copy link
Copy Markdown

netlify bot commented Sep 9, 2024

Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit 3df1980
🔍 Latest deploy log https://app.netlify.com/sites/rspack/deploys/66dedabf27514d00086dc97f

@github-actions github-actions bot added the release: bug fix release: bug related release(mr only) label Sep 9, 2024
@fi3ework fi3ework requested a review from JSerFeng September 9, 2024 11:56
@fi3ework fi3ework enabled auto-merge (squash) September 9, 2024 11:57
@JSerFeng
Copy link
Copy Markdown
Contributor

JSerFeng commented Sep 10, 2024

Note: We can safely use const or let here, because external module has definite runtime condition, so the require won't be wrapped in if statement, which is like

if ("main" == __webpack_require__.j) {
  var foo = require('./foo')
}

So its safe if don't have hoisting here

So LGTM

@fi3ework fi3ework merged commit b1b5504 into web-infra-dev:main Sep 10, 2024
@fi3ework fi3ework deleted the const-con-exter branch September 10, 2024 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

release: bug fix release: bug related release(mr only)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants