Skip to content

fix: concatenate module should use importer's 'strict' for interop #12446

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.

Sign up for GitHub

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

Jump to bottom
Merged
JSerFeng merged 1 commit into from
Dec 15, 2025
Merged

fix: concatenate module should use importer's 'strict' for interop #12446

JSerFeng merged 1 commit into from
Dec 15, 2025

Conversation

@JSerFeng
Copy link
Contributor

@JSerFeng JSerFeng commented Dec 15, 2025

Summary

Concatenated module get_binding should use import module's buildMeta.strictEsm to get export, to make interop correct, the behavior should be the same as which not concatenateed

Related links

Checklist

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

Copilot AI review requested due to automatic review settings December 15, 2025 04:43
@netlify
Copy link

netlify bot commented Dec 15, 2025 •
edited
Loading

✅ Deploy Preview for rspack canceled.

Name Link
🔨 Latest commit f12603c
🔍 Latest deploy log https://app.netlify.com/projects/rspack/deploys/693f91e19122d20008d8985e

@github-actions github-actions bot added team The issue/pr is created by the member of Rspack. release: bug fix release: bug related release(mr only) labels Dec 15, 2025
@JSerFeng JSerFeng enabled auto-merge (squash) December 15, 2025 04:46
Copilot AI reviewed Dec 15, 2025
View reviewed changes
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes a bug in module concatenation where the interop behavior was incorrectly determined by the re-exported module's strictEsm setting instead of the importer module's setting. The fix ensures that when resolving re-exports during module concatenation, the system uses the importer's strict_esm_module flag to determine the correct ES module interop behavior.

  • Updated get_final_binding recursive call to use the current (importer) module's strict_esm_module flag instead of the reexport target's flag
  • Added comprehensive test case demonstrating correct interop behavior with .js (non-strict) and .mjs (strict) file extensions importing from a CommonJS module with __esModule marker

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
crates/rspack_core/src/concatenated_module.rs Fixed recursive get_final_binding call to use importer's strict_esm_module flag for correct interop resolution
tests/rspack-test/configCases/concatenate-modules/interop/rspack.config.js Test configuration enabling module concatenation
tests/rspack-test/configCases/concatenate-modules/interop/index.js Test entry point validating correct interop behavior for both .js and .mjs importers
tests/rspack-test/configCases/concatenate-modules/interop/module.js Non-strict ESM module re-exporting from CommonJS
tests/rspack-test/configCases/concatenate-modules/interop/module.mjs Strict ESM module re-exporting from CommonJS
tests/rspack-test/configCases/concatenate-modules/interop/module-normal.cjs CommonJS module with __esModule marker and both function export and default property

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@github-actions
Copy link
Contributor

github-actions bot commented Dec 15, 2025

Rsdoctor Bundle Diff Analysis

Found 5 project(s) in monorepo.

📁 react-10k

Path: ../build-tools-performance/cases/react-10k/dist/rsdoctor-data.json

📌 Baseline Commit: f4c00dd41e | PR: #12435

Metric Current Baseline Change
📊 Total Size 5.7 MB 5.7 MB 0 B (0.0%)
📄 JavaScript 5.7 MB 5.7 MB 0 B (0.0%)
🎨 CSS 21.0 B 21.0 B 0 B (0.0%)
🌐 HTML 0 B 0 B N/A
📁 Other Assets 0 B 0 B N/A

📦 Download Diff Report: react-10k Bundle Diff

📁 react-5k

Path: ../build-tools-performance/cases/react-5k/dist/rsdoctor-data.json

📌 Baseline Commit: f4c00dd41e | PR: #12435

Metric Current Baseline Change
📊 Total Size 2.7 MB 2.7 MB 0 B (0.0%)
📄 JavaScript 2.7 MB 2.7 MB 0 B (0.0%)
🎨 CSS 21.0 B 21.0 B 0 B (0.0%)
🌐 HTML 0 B 0 B N/A
📁 Other Assets 0 B 0 B N/A

📦 Download Diff Report: react-5k Bundle Diff

📁 react-1k

Path: ../build-tools-performance/cases/react-1k/dist/rsdoctor-data.json

📌 Baseline Commit: f4c00dd41e | PR: #12435

Metric Current Baseline Change
📊 Total Size 823.6 KB 823.6 KB 0 B (0.0%)
📄 JavaScript 823.6 KB 823.6 KB 0 B (0.0%)
🎨 CSS 0 B 0 B N/A
🌐 HTML 0 B 0 B N/A
📁 Other Assets 0 B 0 B N/A

📦 Download Diff Report: react-1k Bundle Diff

📁 ui-components

Path: ../build-tools-performance/cases/ui-components/dist/rsdoctor-data.json

📌 Baseline Commit: f4c00dd41e | PR: #12435

Metric Current Baseline Change
📊 Total Size 2.1 MB 2.1 MB 0 B (0.0%)
📄 JavaScript 2.0 MB 2.0 MB 0 B (0.0%)
🎨 CSS 83.0 KB 83.0 KB 0 B (0.0%)
🌐 HTML 0 B 0 B N/A
📁 Other Assets 0 B 0 B N/A

📦 Download Diff Report: ui-components Bundle Diff

📁 rome

Path: ../build-tools-performance/cases/rome/dist/rsdoctor-data.json

📌 Baseline Commit: f4c00dd41e | PR: #12435

Metric Current Baseline Change
📊 Total Size 984.3 KB 984.3 KB 0 B (0.0%)
📄 JavaScript 984.3 KB 984.3 KB 0 B (0.0%)
🎨 CSS 0 B 0 B N/A
🌐 HTML 0 B 0 B N/A
📁 Other Assets 0 B 0 B N/A

📦 Download Diff Report: rome Bundle Diff

Generated by Rsdoctor GitHub Action

@github-actions
Copy link
Contributor

github-actions bot commented Dec 15, 2025

📦 Binary Size-limit

Comparing f12603c to fix: strip ANSI codes from ignoreWarnings messages (#12435) by neverland

🎉 Size decreased by 256bytes from 48.20MB to 48.20MB (⬇️0.00%)

@codspeed-hq
Copy link

codspeed-hq bot commented Dec 15, 2025

CodSpeed Performance Report

Merging #12446 will not alter performance

Comparing fix/get-binding (f12603c) with main (f4c00dd)

Summary

✅ 17 untouched

@JSerFeng JSerFeng merged commit 93edcb6 into main Dec 15, 2025
61 checks passed
@JSerFeng JSerFeng deleted the fix/get-binding branch December 15, 2025 06:03
@CPunisher CPunisher mentioned this pull request Dec 17, 2025
Merged
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@ahabhgk ahabhgk ahabhgk approved these changes

Assignees

No one assigned

Labels

release: bug fix release: bug related release(mr only) team The issue/pr is created by the member of Rspack.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@JSerFeng @ahabhgk