fix: duplicate variable declarations in ESM output #2239
Conversation
- Added deduplication logic in handle_entry_resources.rs to prevent duplicate variable declarations - Only the first occurrence of an export creates a var declaration - Subsequent exports with the same name only create export statements - Added comprehensive test cases in examples/duplicate-export-fix - Fixes issues with 'export *' combined with named exports
🦋 Changeset detectedLatest commit: bcd775d The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughDeduplicates named ESM exports in plugin_runtime to prevent duplicate variable declarations; adds an example project exercising complex and namespace re-exports, a Farm config, HTML test page, and Vitest end-to-end tests; includes a changeset declaring a patch release for @farmfe/plugin-runtime. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Builder as Builder
participant Runtime as get_export_info_code
participant Info as export_info[]
participant Emitter as CodeEmitter
Builder->>Runtime: generate(export_info, EsModule)
Runtime->>Runtime: sort export_info by rank
Runtime->>Runtime: init seen_names, result_parts
loop each export in export_info
alt export is Default
Runtime->>result_parts: append "export default entry.default || entry"
else export is Named
alt name not in seen_names
Runtime->>result_parts: declare local var for name
Runtime->>result_parts: append export (with alias if import_as)
Runtime->>seen_names: add name
else name already seen
Runtime->>result_parts: append export only (with alias if import_as)
end
else export is CJS entry in ESM path
Runtime->>result_parts: append "export default entry"
end
end
Runtime->>Emitter: join result_parts -> ESM code
Emitter-->>Builder: emit deduplicated ESM bundle
Note right of Runtime: Deduplicate named exports to avoid duplicate var declarations
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (11)
crates/plugin_runtime/src/handle_entry_resources.rs (2)
176-186: Minor: make the sort deterministic and intention-revealingIf ordering matters for reproducibility, prefer an explicitly stable sort (it also clarifies intent for readers).
Apply:
- export_info.sort_by_key(|v| v.rank()); + export_info.sort_by_key(|v| v.rank()); // stable ordering desired + // If non-stable sort is ever used here, switch to sort_by with a tie-breaker.
115-151: Potential incorrect handling of string-named exports
ModuleExportName::Strvalues may not be valid JS identifiers. Generatingvar {name} = ...can yield invalid code. Consider normalizing to a safe identifier (e.g.,__exp_<hash>) and always access with bracket notation.If you adopt the alias approach above, you can emit:
- result_parts.push(format!("var {name}=__farm_entry__.{name};")); + let safe = to_safe_ident(name); // e.g., transform/escape or hash if needed + result_parts.push(format!("var {safe}=__farm_entry__[{key}];", + key = serde_json::to_string(name).unwrap() // quoted string key +)); + // and export { safe as <exported> }Happy to provide a small
to_safe_identhelper if you want to pursue this..changeset/lemon-snakes-jog.md (2)
2-2: Use patch instead of minor for a bugfix.This is a behavior fix without API changes; a patch release seems more appropriate.
-"@farmfe/runtime": minor +"@farmfe/runtime": patch
5-5: Format code terms in backticks.Improves readability in the changelog entry.
-Fix duplicate variable declarations in ESM output when using export \* with named exports +Fix duplicate variable declarations in ESM output when using `export *` with named exportsexamples/duplicate-export-fix/package.json (1)
1-14: Mark the example package as private.Prevents accidental publish from the workspace.
{ "name": "duplicate-export-fix", "version": "1.0.0", + "private": true, "type": "module",examples/duplicate-export-fix/farm.config.mjs (1)
1-16: Resolve paths relative to the config file to avoid CWD sensitivity.Helps when running builds/tests from different working directories.
-import { defineConfig } from '@farmfe/core'; +import { defineConfig } from '@farmfe/core'; +import { fileURLToPath } from 'node:url'; +import { dirname, resolve } from 'node:path'; + +const __filename = fileURLToPath(import.meta.url); +const __dirname = dirname(__filename); export default defineConfig({ compilation: { input: { - complex: './src/complex-entry.js', - namespace: './src/namespace-entry.js' + complex: resolve(__dirname, './src/complex-entry.js'), + namespace: resolve(__dirname, './src/namespace-entry.js') }, output: { - path: './dist', + path: resolve(__dirname, './dist'), format: 'esm' }, minify: false, sourcemap: false } });examples/duplicate-export-fix/index.html (1)
28-31: Avoid innerHTML; prefer textContent for test messages.Reduces risk of accidental HTML injection in examples.
- results.innerHTML = '<p style="color: green;">✓ All exports are working correctly!</p>'; + results.textContent = '✓ All exports are working correctly!'; + results.style.color = 'green'; } else { - results.innerHTML = '<p style="color: red;">✗ Some exports are not working</p>'; + results.textContent = '✗ Some exports are not working'; + results.style.color = 'red';examples/duplicate-export-fix/e2e.spec.ts (4)
24-27: Escape identifier and tighten regex to avoid false positives.Prevents regex surprises and matches
var <id> =with whitespace tolerance.Apply this diff:
- const countVarDeclarations = (content: string, varName: string) => { - const regex = new RegExp(`var ${varName}=`, 'g'); - return (content.match(regex) || []).length; - }; + const countVarDeclarations = (content: string, varName: string) => { + const escaped = varName.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + const regex = new RegExp(`\\bvar\\s+${escaped}\\s*=`, 'gm'); + return (content.match(regex) || []).length; + };
20-21: Use path.join segments for cross-platform paths.Minor, but avoids mixed separators.
Apply this diff:
- const complexOutput = readFileSync(join(examplePath, 'dist/complex.js'), 'utf-8'); - const namespaceOutput = readFileSync(join(examplePath, 'dist/namespace.js'), 'utf-8'); + const complexOutput = readFileSync(join(examplePath, 'dist', 'complex.js'), 'utf-8'); + const namespaceOutput = readFileSync(join(examplePath, 'dist', 'namespace.js'), 'utf-8');
39-42: Make export checks resilient to formatting.Use regex to ignore whitespace differences.
Apply this diff:
- expect(complexOutput).toContain('export { shared }'); - expect(namespaceOutput).toContain('export { formatDate }'); - expect(namespaceOutput).toContain('export { formatTime }'); + expect(complexOutput).toMatch(/export\s*{\s*shared\s*}/); + expect(namespaceOutput).toMatch(/export\s*{\s*formatDate\s*}/); + expect(namespaceOutput).toMatch(/export\s*{\s*formatTime\s*}/);
49-63: Attach console listeners before waiting to catch early errors.Ensures syntax/runtime errors during initial load are recorded.
Apply this diff:
- // Wait for the test results div - await page.waitForSelector('#test-results', { timeout: 10000 }); - - // Check that all exports are working - const resultsText = await page.$eval('#test-results', el => el.textContent); - expect(resultsText).toContain('All exports are working correctly!'); - - // Verify no JavaScript errors occurred - const consoleErrors: string[] = []; - page.on('console', msg => { - if (msg.type() === 'error') { - consoleErrors.push(msg.text()); - } - }); + // Capture console errors early + const consoleErrors: string[] = []; + page.on('console', (msg) => { + if (msg.type() === 'error') { + consoleErrors.push(msg.text()); + } + }); + + // Wait for the test results div + await page.waitForSelector('#test-results', { timeout: 10000 }); + + // Check that all exports are working + const resultsText = await page.$eval('#test-results', (el) => el.textContent); + expect(resultsText).toContain('All exports are working correctly!');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
.changeset/lemon-snakes-jog.md(1 hunks)crates/plugin_runtime/src/handle_entry_resources.rs(1 hunks)examples/duplicate-export-fix/.gitignore(1 hunks)examples/duplicate-export-fix/e2e.spec.ts(1 hunks)examples/duplicate-export-fix/farm.config.mjs(1 hunks)examples/duplicate-export-fix/index.html(1 hunks)examples/duplicate-export-fix/package.json(1 hunks)examples/duplicate-export-fix/src/complex-entry.js(1 hunks)examples/duplicate-export-fix/src/lib/library1.js(1 hunks)examples/duplicate-export-fix/src/lib/library2.js(1 hunks)examples/duplicate-export-fix/src/namespace-entry.js(1 hunks)examples/duplicate-export-fix/src/shared/common.js(1 hunks)examples/duplicate-export-fix/src/shared/utils.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
examples/duplicate-export-fix/e2e.spec.ts (2)
scripts/build.mjs (1)
examplePath(52-52)e2e/vitestSetup.ts (1)
startProjectAndTest(87-178)
examples/duplicate-export-fix/src/namespace-entry.js (1)
crates/compiler/tests/fixtures/script/import_equals/output.js (1)
utils(78-78)
🪛 ast-grep (0.38.6)
examples/duplicate-export-fix/e2e.spec.ts
[warning] 24-24: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(var ${varName}=, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (9)
examples/duplicate-export-fix/.gitignore (1)
1-5: LGTM for example ignoresCovers typical Node artifacts for the example app.
examples/duplicate-export-fix/src/shared/common.js (1)
1-3: LGTM: clear, minimal fixturesSimple constants are fine for exercising export generation.
examples/duplicate-export-fix/src/shared/utils.js (1)
1-4: LGTM: concise helpers for namespace export caseSufficient for testing re-export/namespace behavior.
crates/plugin_runtime/src/handle_entry_resources.rs (1)
223-239: CJS branch ok, but consider symmetry testsCJS-to-ESM default interop is handled via
export default entry;. Add a test ensuringimport defaultfrom a CJS entry works as expected and that no named exports are generated for CJS.You can extend the example or add a new one where the entry is CJS and assert that the built ESM only has a default export.
examples/duplicate-export-fix/src/lib/library1.js (1)
1-4: LGTM — re-export pattern correctly exercises the duplicate-decl scenario.examples/duplicate-export-fix/src/lib/library2.js (1)
2-4: LGTM: Re-export + alias pattern correctly exercises the dedup path.This fixture is minimal and appropriate for the scenario.
examples/duplicate-export-fix/src/namespace-entry.js (1)
2-5: LGTM: Namespace + star re-exports cover the intended collision cases.Good coverage for both
export * asandexport *interactions.examples/duplicate-export-fix/src/complex-entry.js (1)
3-6: LGTM: Complex re-exports create the expected duplicate names for testing.This should reliably hit the deduplication logic.
examples/duplicate-export-fix/e2e.spec.ts (1)
14-17: Keep using pnpm for the build. pnpm is present in PATH (/usr/local/bin/pnpm), so switching tonpm run buildor adding a fallback isn’t necessary.
|
|
||
| // For ESM format, deduplicate named exports to prevent duplicate variable declarations | ||
| if matches!(context.config.output.format, ModuleFormat::EsModule) { | ||
| let mut seen_names = HashSet::new(); | ||
| let mut result_parts = Vec::new(); | ||
|
|
||
| for export in export_info.iter() { | ||
| match export { | ||
| ExportInfoOfEntryModule::Default => { | ||
| result_parts.push("export default entry.default || entry;".to_string()); | ||
| }, | ||
| ExportInfoOfEntryModule::Named { name, import_as } => { | ||
| // Only add if we haven't seen this variable name before | ||
| if seen_names.insert(name.clone()) { | ||
| if let Some(import_as) = import_as { | ||
| result_parts.push(format!("var {name}=entry.{name};export {{ {name} as {import_as} }};")); | ||
| } else { | ||
| result_parts.push(format!("var {name}=entry.{name};export {{ {name} }};")); | ||
| } | ||
| } else { | ||
| // If we've seen this name before, only add the export without the var declaration | ||
| if let Some(import_as) = import_as { | ||
| result_parts.push(format!("export {{ {name} as {import_as} }};")); | ||
| } else { | ||
| result_parts.push(format!("export {{ {name} }};")); | ||
| } | ||
| } | ||
| } else { | ||
| match context.config.output.format { | ||
| ModuleFormat::CommonJs => format!("module.exports.{name} = entry.{name};"), | ||
| ModuleFormat::EsModule => format!("var {name}=entry.{name};export {{ {name} }};"), | ||
| } | ||
| }, | ||
| ExportInfoOfEntryModule::CJS => { | ||
| result_parts.push("export default entry;".to_string()); | ||
| } | ||
| } | ||
| ExportInfoOfEntryModule::CJS => match context.config.output.format { | ||
| ModuleFormat::CommonJs => "module.exports = entry;".to_string(), | ||
| ModuleFormat::EsModule => "export default entry;".to_string(), | ||
| }, | ||
| }) | ||
| .collect::<Vec<String>>() | ||
| .join("") | ||
| } | ||
|
|
||
| result_parts.join("") | ||
| } else { | ||
| // For CommonJS, keep the original logic | ||
| export_info | ||
| .iter() | ||
| .map(|export| match export { | ||
| ExportInfoOfEntryModule::Default => "module.exports = entry.default || entry;".to_string(), | ||
| ExportInfoOfEntryModule::Named { name, import_as } => { | ||
| if let Some(import_as) = import_as { | ||
| format!("module.exports.{import_as} = entry.{name};") | ||
| } else { | ||
| format!("module.exports.{name} = entry.{name};") | ||
| } | ||
| } | ||
| ExportInfoOfEntryModule::CJS => "module.exports = entry;".to_string(), | ||
| }) | ||
| .collect::<Vec<String>>() | ||
| .join("") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Still emits duplicate export statements; also risks shadowing the entry binding
- The new logic dedupes var declarations, but still emits repeated
export { name }for subsequent occurrences of the same export, which is a SyntaxError in ESM (duplicate export of name). - If a module exports a symbol literally named
entry, the generatedvar entry = entry.entry;will reassign yourentryobject and break subsequent exports.
Recommend:
- Deduplicate both declarations and export statements.
- Use a safe alias for the exports object to avoid collisions with user exports named
entry.
Proposed patch:
@@
- if matches!(context.config.output.format, ModuleFormat::EsModule) {
- let mut seen_names = HashSet::new();
- let mut result_parts = Vec::new();
-
- for export in export_info.iter() {
+ if matches!(context.config.output.format, ModuleFormat::EsModule) {
+ let mut declared = HashSet::new(); // names we've created locals for
+ let mut exported = HashSet::new(); // names we've actually exported
+ let mut result_parts = Vec::new();
+ // avoid shadowing if user exports a symbol named `entry`
+ result_parts.push("const __farm_entry__ = entry;".to_string());
+
+ for export in export_info.iter() {
match export {
ExportInfoOfEntryModule::Default => {
- result_parts.push("export default entry.default || entry;".to_string());
+ result_parts.push("export default entry.default || entry;".to_string());
},
ExportInfoOfEntryModule::Named { name, import_as } => {
- // Only add if we haven't seen this variable name before
- if seen_names.insert(name.clone()) {
- if let Some(import_as) = import_as {
- result_parts.push(format!("var {name}=entry.{name};export {{ {name} as {import_as} }};"));
- } else {
- result_parts.push(format!("var {name}=entry.{name};export {{ {name} }};"));
- }
- } else {
- // If we've seen this name before, only add the export without the var declaration
- if let Some(import_as) = import_as {
- result_parts.push(format!("export {{ {name} as {import_as} }};"));
- } else {
- result_parts.push(format!("export {{ {name} }};"));
- }
- }
+ // declare once
+ if declared.insert(name.clone()) {
+ result_parts.push(format!("var {name}=__farm_entry__.{name};"));
+ }
+ // export once
+ let exported_name = import_as.as_ref().unwrap_or(name);
+ if exported.insert(exported_name.clone()) {
+ if let Some(import_as) = import_as {
+ result_parts.push(format!("export {{ {name} as {import_as} }};"));
+ } else {
+ result_parts.push(format!("export {{ {name} }};"));
+ }
+ }
},
ExportInfoOfEntryModule::CJS => {
result_parts.push("export default entry;".to_string());
}
}
}
result_parts.join("")This prevents duplicate exports and avoids entry shadowing.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In crates/plugin_runtime/src/handle_entry_resources.rs around lines 187 to 239,
the ESM branch currently only deduplicates var declarations but still emits
duplicate "export { name }" statements (causing SyntaxError) and risks shadowing
a user export named `entry`; change the generation so you deduplicate both the
declaration and the export by tracking emitted export names (skip emitting any
export if the name was already emitted), and stop referencing the input `entry`
identifier directly — read properties from a safe internal alias (e.g., rename
the incoming binding to something like __entry or assign const __entry = entry)
so we never generate `var entry = entry.entry` or reassign the original; ensure
default/CJS handling remains equivalent and use one-time declaration + export
emission per unique name.
|
Thanks! I'll merge it when all checks passed. BTW, you can set output.targetEnv to library, which would produce pure esm bundle without complext farm runtime, current way to bundle scripts is based on farm runtime |
|
Frankly, it's good to hear that there's a solution without having to go through such complex processes, because I was just trying to help solve a problem I was having while using it, and I didn't know about targetEnv :) Still, if it might be helpful to someone else, I'd be happy to contribute. |
|
Thank you very much for your contribution, could you help resolve the merge conflicts |
- Added .gitignore to exclude build outputs and node_modules - Keeps repository clean from generated files
brkmustu
force-pushed
the
dublicate-export-generation
branch
from
b238699 to
bcd775d
Compare
2 participants
Description
Fixes duplicate variable declarations in ESM output when using
export *combined with named exports.Problem
When multiple modules re-export the same identifiers using different patterns (e.g.,
export *andexport { name }), Farm was generating duplicate variable declarations in the final ESM output: