fix: require.resolve() replaced as require()#12773
fix: require.resolve() replaced as require()#12773LingyuCoder merged 9 commits intoweb-infra-dev:mainfrom
require.resolve() replaced as require()#12773Conversation
✅ Deploy Preview for rspack canceled.Built without sensitive environment variables
|
require.resolve() replaced as require()
require.resolve() replaced as require()require.resolve() replaced as require()
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where require.resolve() and require.resolveWeak() were incorrectly being replaced as require() calls, and prevents transpilation when require is a local variable rather than the global CommonJS require.
Changes:
- Fixed identifier evaluation for
require.resolveandrequire.resolveWeakto preserve correct identifier names - Added
should_process_resolve()check to detect whenrequireis a local variable and skip CommonJS transformation - Added test case to verify the fix works correctly
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/rspack_plugin_javascript/src/parser_plugin/common_js_imports_parse_plugin.rs |
Fixed bug where require.resolve and require.resolveWeak were incorrectly evaluated with root_info as "require" instead of their actual names; added should_process_resolve() to check if require is a local variable; integrated check into resolve handlers |
tests/rspack-test/normalCases/require/issue-10479/require.js |
Creates a local require variable using createRequire to test the fix |
tests/rspack-test/normalCases/require/issue-10479/index.js |
Verifies that require.resolve() is not transformed when require is a local variable |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
LGTM, thanks |
|
Thank you! |
* fix issue 10479 * fix issue 10479 * fix issue 10479 * revert incorrect changes * fix `require.resolve()`` replaced as `require()` * check if `require` is a local variable * fix test * fix test
* fix issue 10479 * fix issue 10479 * fix issue 10479 * revert incorrect changes * fix `require.resolve()`` replaced as `require()` * check if `require` is a local variable * fix test * fix test
Summary
*
require.resolve()andrequire.resolveWeakshould not be replaced asrequire()requireis a local variable, should not be transpiled as commonjs importRelated links
#10479
Checklist