revert: refactor: remove support for nested importModule usage (#12111) #12165
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This reverts commit a2ba295.
✅ Deploy Preview for rspack canceled.
|
github-actions
bot
added
the
team
stormslowly
changed the title
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR reverts commit a2ba295 which had removed support for nested importModule usage in loaders. The revert re-enables nested importModule calls by changing the error handling from "not supported" to proper circular dependency detection.
Key changes:
- Re-enables nested
importModuleusage in loaders by removing the early rejection - Updates error messages to report "circular build dependency" instead of "nested calls not supported"
- Adds new test cases for cache behavior with loader import modules
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| crates/rspack_core/src/compilation/make/module_executor/module_tracker.rs | Removes is_module_building check and renames is_entry_running to is_running |
| crates/rspack_core/src/compilation/make/module_executor/entry.rs | Removes nested importModule check and updates function call to is_running |
| tests/rspack-test/normalCases/errors/import-module-cycle/index.js | Updates error expectations from "nested calls not supported" to "circular build dependency" |
| tests/rspack-test/normalCases/errors/import-module-cycle-multiple/index.js | Updates expected error outputs to show deeper cycle detection |
| tests/rspack-test/configCases/loader-import-module/recursive-import-module/errors.js | Removes error expectations for nested importModule (now allowed) |
| tests/rspack-test/configCases/loader-parallel-import-module/recursive-import-module/errors.js | Removes error expectations for nested importModule (now allowed) |
| tests/rspack-test/watchCases/cache/loader-import-module/* | Adds new test suite for cache behavior with loader import modules |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,2 @@ | |||
|
|
|||
| module.exports = () => "FIXME: panic" | |||
There was a problem hiding this comment.
The test filter contains a 'FIXME: panic' message, indicating the test is currently disabled or broken. This should be addressed before merging the revert to ensure the test properly validates the restored nested importModule functionality.
Suggested change
| module.exports = () => "FIXME: panic" | |
| module.exports = () => true |
| import a from "./a.generate-json.js"; | ||
| import { value as unrelated } from "./unrelated"; | ||
|
|
||
| it("should have to correct values and validate on change", () => { |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
Suggested change
| it("should have to correct values and validate on change", () => { | |
| it("should have to correct values and validate on change", () => { | |
| expect(a).not.toBeUndefined(); |
| import a from "./a.generate-json.js"; | ||
| import { value as unrelated } from "./unrelated"; | ||
|
|
||
| it("should have to correct values and validate on change", () => { |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
Suggested change
| it("should have to correct values and validate on change", () => { | |
| it("should have to correct values and validate on change", () => { | |
| if (typeof a === "undefined") { | |
| throw new Error('Imported "a" from "./a.generate-json.js" is undefined. Please ensure the module exports the expected object.'); | |
| } |
Comment on lines
+8
to
+13
| expect(a.nested.value).toBe(step < 3 ? 42 : 24); | ||
| expect(a.nested.a).toBe(step < 3 ? "a" : undefined); | ||
| expect(a.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.nested.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.c).toBe(step < 1 ? undefined : "c"); | ||
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
Suggested change
| expect(a.nested.value).toBe(step < 3 ? 42 : 24); | |
| expect(a.nested.a).toBe(step < 3 ? "a" : undefined); | |
| expect(a.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.nested.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.c).toBe(step < 1 ? undefined : "c"); | |
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); | |
| expect(a.nested?.value).toBe(step < 3 ? 42 : 24); | |
| expect(a.nested?.a).toBe(step < 3 ? "a" : undefined); | |
| expect(a.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.nested?.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.c).toBe(step < 1 ? undefined : "c"); | |
| expect(a.nested?.c).toBe(step < 1 || step >= 3 ? undefined : "c"); |
Comment on lines
+8
to
+13
| expect(a.nested.value).toBe(step < 3 ? 42 : 24); | ||
| expect(a.nested.a).toBe(step < 3 ? "a" : undefined); | ||
| expect(a.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.nested.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.c).toBe(step < 1 ? undefined : "c"); | ||
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
Suggested change
| expect(a.nested.value).toBe(step < 3 ? 42 : 24); | |
| expect(a.nested.a).toBe(step < 3 ? "a" : undefined); | |
| expect(a.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.nested.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.c).toBe(step < 1 ? undefined : "c"); | |
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); | |
| expect(a.nested?.value).toBe(step < 3 ? 42 : 24); | |
| expect(a.nested?.a).toBe(step < 3 ? "a" : undefined); | |
| expect(a.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.nested?.b).toBe(step < 1 ? "b" : undefined); | |
| expect(a.c).toBe(step < 1 ? undefined : "c"); | |
| expect(a.nested?.c).toBe(step < 1 || step >= 3 ? undefined : "c"); |
| expect(a.nested.a).toBe(step < 3 ? "a" : undefined); | ||
| expect(a.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.nested.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.c).toBe(step < 1 ? undefined : "c"); |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
| expect(a.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.nested.b).toBe(step < 1 ? "b" : undefined); | ||
| expect(a.c).toBe(step < 1 ? undefined : "c"); | ||
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
Suggested change
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); | |
| expect(a.nested?.c).toBe(step < 1 || step >= 3 ? undefined : "c"); |
| expect(a.c).toBe(step < 1 ? undefined : "c"); | ||
| expect(a.nested.c).toBe(step < 1 || step >= 3 ? undefined : "c"); | ||
| if (step !== 0) { | ||
| expect(STATE.random === a.random).toBe(step === 2); |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
| if (step !== 0) { | ||
| expect(STATE.random === a.random).toBe(step === 2); | ||
| } | ||
| STATE.random = a.random; |
There was a problem hiding this comment.
The base expression of this property access is always undefined.
Comment on lines
+2
to
+3
| import { value as unrelated } from "./unrelated"; | ||
|
|
There was a problem hiding this comment.
Unused import unrelated.
Suggested change
| import { value as unrelated } from "./unrelated"; |
Contributor
📦 Binary Size-limit
🙈 Size remains the same at 48.09MB |
stormslowly
approved these changes
Nov 12, 2025
CodSpeed Performance ReportMerging #12165 will not alter performanceComparing Summary
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This reverts commit a2ba295.
Summary
Fix the ecosystem CI: https://github.com/web-infra-dev/rspack/actions/runs/19288926831/job/55155671354
Related links
Checklist