feat: Farm v2.0 dev #1835
feat: Farm v2.0 dev #1835
Conversation
* fix: clippy issue * fix: example panic when start * fix: runtime should not be emitted twice for a single bundle
fix: tree shake, HMR and source map issues
* fix(schema): invalid server schema with types * chore: optimize types and schema --------- Co-authored-by: ADNY <66500121+ErKeLost@users.noreply.github.com>
WalkthroughThe pull request brings widespread refactoring across the repository. Major changes include a comprehensive streamlining of module export interop in JavaScript/TypeScript files—removing legacy helper functions and introducing a simplified Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Application Code
participant CommonJs as __commonJs Function
participant Module as Module Object
Caller->>CommonJs: Call __commonJs(mod)
CommonJs->>Module: Check if module already initialized
alt Module exists
Module-->>Caller: Return module.exports
else Module not exists
CommonJs->>Module: Initialize module object<br>and execute mod function
Module-->>CommonJs: Populated module.exports
CommonJs-->>Caller: Return module.exports
end
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Pro Plan! Simply use the command ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (74)
crates/compiler/tests/fixtures/bundle/library/reexport/esm/star/output.js (1)
11-12: Improved export approach enhances code readability and maintainability.The simplified export syntax combining named and default exports into a single statement is much cleaner than the previous approach that used utility functions for module interop. This aligns with the broader refactoring effort in Farm v2.0 to streamline module handling.
Consider adding a brief comment explaining that this is re-exporting from the foo module, which would enhance maintainability, especially for developers who are new to the codebase:
// module_id: index.ts +// Re-export named exports and default from foo.ts export { bar, foo, foo_default as default };crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/output.js (1)
44-47: Consider breaking down complex import expressions.Line 46 contains multiple operations in a single statement, which may reduce readability. While functional, it would be more maintainable to split this into separate variable declarations.
- var lodash = _interop_require_default(bundle2_cjs()).default, merge = bundle2_cjs()["merge"]; + var bundleExports = bundle2_cjs(); + var lodash = _interop_require_default(bundleExports).default; + var merge = bundleExports["merge"];.changeset/dull-apes-raise.md (1)
4-6: Clarify Commit Message and BREAKING CHANGE NoteThe commit message "feat: add farm dts generate rust plugin" succinctly describes the feature addition. However, the PR description mentions a BREAKING CHANGE. Please verify whether any backward‐incompatible changes should also be documented (e.g., by including a "BREAKING CHANGE:" section) or if the breaking change is addressed elsewhere.
crates/compiler/tests/fixtures/bundle/library/hybrid/esm_export_cjs/output.cjs.js (2)
3-19: New__commonJsfunction implements effective module caching.The function provides a clean way to handle CommonJS modules, ensuring they're only executed once with proper exports caching. However, there's a potential issue in the module resolution logic.
The logic at line 15 assumes that when
modis an object, it has at least one property that is a function. Consider adding validation or a more robust approach:if (typeof mod === "function") { mod(module, module.exports); } else { - mod[Object.keys(mod)[0]](module, module.exports); + const key = Object.keys(mod)[0]; + if (key && typeof mod[key] === 'function') { + mod[key](module, module.exports); + } else { + throw new Error('Invalid module format'); + } }
54-56: Duplicate__esModuleproperty setting.The
__esModuleproperty is set twice - once in the farm_runtime.js section (lines 35-37) and again here. This is redundant and could be confusing.-Object.defineProperty(exports, "__esModule", { - value: true -});Since the property is already set in the farm_runtime.js section, this second definition is unnecessary unless there's a specific reason to reset it.
crates/compiler/tests/fixtures/bundle/library/hybrid/normal/output.cjs.js (2)
3-19: Implementation of CommonJS module loaderThis new
__commonJsfunction creates a factory for CommonJS modules with lazy initialization and caching. The implementation follows the standard CommonJS pattern where modules are executed once and their exports are cached for subsequent imports.Consider handling edge cases in line 15 where
Object.keys(mod)[0]assumes the first key is the function to call. If the object has multiple keys or non-function values, this could cause unexpected behavior.- mod[Object.keys(mod)[0]](module, module.exports); + const key = Object.keys(mod)[0]; + if (typeof mod[key] === 'function') { + mod[key](module, module.exports); + } else { + throw new Error(`Expected module[${key}] to be a function`); + }
61-66: Debug statement in production codeThere's a console.log statement that appears to be for debugging purposes.
Consider removing the console.log statement in line 62 as it may not be intended for production code:
- console.log(fs.read, os.cpus);crates/compiler/tests/fixtures/library/reexport/basic/output.js (1)
1-72: Consider adding more descriptive commentsWhile this is a test fixture, adding some comments to explain the generated namespace objects and how they correspond to the original module structure would improve readability and make the test case more self-documenting.
crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/index.ts (1)
27-34: Variable shadowing in computed function.The parameter
barin thecomputedfunction shadows the outer variablebardefined on line 27. While this works as expected and may be intentional for testing, it could be confusing in production code.const bar = 2; -function computed(bar = 1) { +function computed(barParam = 1) { return { - [bar]: 123, + [barParam]: 123, [defaultFoo]: 234, } }crates/compiler/tests/bundle.rs (3)
49-50: Target environment change and auto_external_failed_resolve setting.The target environment has been changed from browser to node, and a new setting for auto-externalizing failed resolves has been added.
Consider adding a comment explaining what
auto_external_failed_resolvedoes and why it's being enabled to improve code maintainability.
83-85: Consider if single_bundle_test is necessary.This function is a simple wrapper around
testwith aNoneparameter.While this wrapper provides naming clarity, it could be replaced with direct calls to
test(file, crate_path, None)to reduce function count, unless the naming is important for test organization.
100-100: Consider removing or uncommenting this test case.There's a commented-out testing macro that isn't being used.
Either uncomment this test if it's needed to cover an important scenario, or remove it completely to keep the codebase clean.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/output.js (2)
13-15: Variable naming inconsistency and renaming strategyThe bundled code uses a numbered suffix strategy for handling naming conflicts:
bundle_str1becomesbundle_str1$1(line 13)index_foobecomesindex_foo$1(line 34)While this prevents name collisions, it can make the code harder to follow. Consider documenting this naming convention for developers working with the bundled output.
Also applies to: 33-40
40-41: Multiple export statements could be consolidatedThe module uses multiple export statements at the end:
export { index_bar, index_foo$1 as index_foo, index_default as default }; export { foo_default, foo_str1, foo_str2 };These could be consolidated into a single export statement for better readability:
export { index_bar, index_foo$1 as index_foo, index_default as default, foo_default, foo_str1, foo_str2 };crates/compiler/tests/fixtures/external/browser/object/cjs/output.js (1)
2-18: Consider validatingmodobject properties and adding error handling
The__commonJsfunction unifies module loading under a single closure, which is concise. However, ifmodis neither a function nor a single-key object, this could lead to runtime errors. Verifying the shape ofmodor handling multiple keys gracefully would make the code more robust.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/star/output.js (2)
3-19: Consider adding checks for object shape in__commonJs.Currently, line 15 invokes the first key of
modwithout verifying its shape or whether multiple keys exist. This could result in unexpected behavior ifmodcontains additional properties or ifmod[Object.keys(mod)[0]]isn't callable. Adding a basic check or fallback could improve resilience.For example:
} else { - mod[Object.keys(mod)[0]](module, module.exports); + const keys = Object.keys(mod); + if (keys.length > 0 && typeof mod[keys[0]] === 'function') { + mod[keys[0]](module, module.exports); + } else { + console.warn("[__commonJs]: No valid export function found for provided mod object keys:", keys); + } }
37-42: Use a safer property iteration strategy in_exportfunction.Using
for (var name in all)can inadvertently capture inherited enumerable properties ifallis not a simple object. Prefer a pattern likeObject.keys(all).forEach(...)to avoid potential edge cases.-function _export(target, all) { - for(var name in all)Object.defineProperty(target, name, { +function _export(target, all) { + Object.keys(all).forEach((name) => Object.defineProperty(target, name, { enumerable: true, get: all[name] - }); + })); }crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/output.js (2)
15-15: Consider making the object-based module handling more robust.This line assumes the first key in the module object is the function to execute. While it works for the current test case, it might be error-prone if the object structure changes.
- mod[Object.keys(mod)[0]](module, module.exports); + const moduleKey = Object.keys(mod)[0]; + if (typeof mod[moduleKey] === 'function') { + mod[moduleKey](module, module.exports); + } else { + console.warn(`Module ${moduleKey} is not a function`); + }
45-46: Empty module ID comment.The comment indicates a module
foo.tsbut no actual code follows. This might be intentional for testing, but could confuse readers. Consider adding a comment explaining why this empty module ID exists.// module_id: foo.ts + // This is an intentionally empty module for testing purposescrates/compiler/tests/fixtures/bundle/library/hybrid/normal/output.js (1)
61-61: Review usage offs.read.
fs.readis a low-level function that relies on file descriptors. If your intention is to read file contents, you might consider usingfs.readFileorfs.readFileSync.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/star/output.cjs.js (3)
3-19: Consider renaming themodulevariable to avoid confusion with Node.js built-ins.
Using a local variable namedmodulecan cause confusion, especially in a Node.js environment. Although it's valid in this scoped bundler context, renaming it to something likewrappedModuleor similar would enhance clarity.
36-36: Be mindful of using__proto__assignment.
Although setting__proto__tonull(equivalent toObject.create(null)) is a common bundler pattern to create a dictionary-like object, it can sometimes lead to unexpected behavior. Review carefully to ensure there's no performance impact or undesired prototype chain interactions.
104-104:foo_nsappears to be unused.
Iffoo_nsis not used later in this module, it can be removed to simplify the code.crates/compiler/tests/fixtures/external/node/hosting/import/output.js (1)
2-18: Evaluate usage ofmod[Object.keys(mod)[0]].
Ifmodcontains multiple or zero keys, this approach risks invoking the wrong function or causing runtime errors. Consider ensuring thatmodreliably contains the function to execute or adjusting the logic to explicitly look up the correct property.// Example suggestion (rename 'main' to your intended function property): -if (typeof mod === "function") { - mod(module, module.exports); -} else { - mod[Object.keys(mod)[0]](module, module.exports); -} +if (typeof mod === "function") { + mod(module, module.exports); +} else if (mod.main) { + mod.main(module, module.exports); +} else { + throw new Error("No valid entry point found in mod."); +}crates/compiler/tests/fixtures/external/node/normala/esm/output.js (1)
2-18: Handle the first-key invocation logic carefully.
As in other parts of the code, callingmod[Object.keys(mod)[0]]assumes a single key. If the object shape changes, this could cause errors. Verify thatmodalways follows this pattern, or add a safeguard for unexpected shapes.crates/compiler/tests/fixtures/external/browser/normal/cjs/output.js (1)
2-18: Consider renaming or explicitly referencing the function rather than usingObject.keys.
This ensures maintainability if more keys appear inmod.crates/compiler/tests/fixtures/external/node/normala/cjs/output.js (1)
2-18: Consider adding fallback and error handling logic for module initialization.
Ifmodis an object without any properties or if the first property is not a valid function,Object.keys(mod)[0]may fail at runtime. Adding a small check can help prevent confusing errors and improve reliability.You could handle unexpected structures gracefully, for example:
-} else { - mod[Object.keys(mod)[0]](module, module.exports); -} +} else if (typeof mod === "object") { + const keys = Object.keys(mod); + if (keys.length && typeof mod[keys[0]] === "function") { + mod[keys[0]](module, module.exports); + } else { + throw new Error("Invalid module initializer."); + } +} else { + throw new Error("Unsupported module format."); +}crates/compiler/tests/fixtures/external/browser/normal/esm/output.js (1)
2-18: Add checks for invalid module definitions to improve robustness.
Similar to the Node version, callingmod[Object.keys(mod)[0]]risks undefined behavior if the module is improperly structured. An explicit check prevents runtime errors and improves maintainability.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/reexport.ts (1)
1-3: Commented out code included in the fileThere are commented-out import/export statements at the top of this new file. Consider removing these comments if they're no longer needed, as they might confuse future developers about implementation details.
crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/named/output.js (1)
51-51: Consider using destructuring for readability.
Currently, properties are accessed via bracket notation. Switching to destructuring is a minor improvement for clarity.-var bar = foo_cjs()["bar"], foo = foo_cjs()["foo"]; +const { bar, foo } = foo_cjs();crates/compiler/tests/fixtures/partial_bundling/basic/output.js (2)
2-3: Consider usingletorconstfor local variable declarations.
Whilevarworks here, block-scoped declarations can help reduce accidental reassignments and improve clarity.
19-20: Naming consideration forindex_js_cjs.
If this function is a primary export, you might rename it for clarity—e.g.,initRuntimeIndexCjs.crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/esm.ts (1)
2-2: Missing semicolon for consistent style.Add a semicolon after the number 19 to maintain consistent style with line 1.
-export const esmAge = 19 +export const esmAge = 19;crates/compiler/tests/fixtures/bundle/library/hybrid/esm_export_cjs/output.js (1)
3-19: Simplified CommonJS module handling with__commonJsfunctionThis function provides a cleaner approach to handling CommonJS modules by implementing a module cache mechanism and supporting both function-style and object-style module definitions. The implementation is well-structured and handles the module execution context properly.
This simplification reduces bundle size and improves code readability by replacing multiple interop helper functions mentioned in the summary.
crates/compiler/src/update/diff_and_patch_module_graph/test_diff_module_deps.rs (1)
37-37: Consistent usage of HashSet::from_iter and HashMap::from_iter throughout the test codeThe code now consistently uses
from_iterinstead offromfor initializing HashSet and HashMap instances. This standardization improves code readability and maintainability while maintaining the same functionality.While not impacting functionality, this change provides a more consistent approach to collection initialization throughout the codebase, which is beneficial for long-term maintenance.
Also applies to: 105-109, 160-161, 215-215, 291-295, 367-368, 415-416, 454-456, 494-496, 540-545
crates/compiler/tests/fixtures/bundle/library/hybrid/hybrid_entry_should_reexport/output.js (1)
20-25: Minor naming refinement for_nodeRequire.
Consider renaming_nodeRequireto a more descriptive name (e.g.,nodeRequireorrequireNodeModule) for clarity. Otherwise, this code is fine for bridging Node's require.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (1)
53-53: Use of.into_keys().collect()is valid.
Cloning the entry keys to create a vector is a reasonable approach for subsequent graph processing. This pattern is repeated in multiple places, so consider extracting a helper function if needed for consistency.Also applies to: 179-179
crates/compiler/src/generate/finalize_resources.rs (1)
62-172:handle_entry_resourcefunction structure appears well-encapsulated.
The new function covers:
- Gathering runtime code from existing resources.
- Constructing and passing a
PluginHandleEntryResourceHookParam.- Inserting updated resources back into the map.
- Handling dynamic resources for each entry module.
Potential suggestions:
- Error boundary: If
entry_resource_namereferences a missing resource pot, it panics. Consider providing a more detailed error message or structured error to make debugging simpler.- Runtime injection: The
emit_runtimeflag toggling resource emission is well-placed. Consider logging or a debug statement when runtime re-injection is triggered, to aid troubleshooting.All changes appear logically consistent. Ensure thorough testing, especially around edge cases such as missing resources or unusual module group configurations.
crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
2-18: Streamlined module initialization via__commonJs.
Replacing older interop functions with__commonJsyields a more concise pattern for handling module exports. The closure-based approach is straightforward and avoids repeated checks.However, consider:
- Guarding against non-function
modinputs if there's any possibility of user-supplied modules.- Confirming
_msufficiency for any diverse import scenarios.Overall, this simplifies loading logic while retaining modular clarity.
crates/compiler/src/build/mod.rs (3)
121-129: Consider using a HashSet for improved performance.
Currently,last_fail_module_ids.contains(&id)runs in O(n) per check. Convertinglast_fail_module_idsto aHashSetwould provide faster lookups, especially for large numbers of errors.fn set_last_fail_module_ids(&self, errors: &[CompilationError]) { let mut last_fail_module_ids = self.last_fail_module_ids.lock(); last_fail_module_ids.clear(); - for id in get_module_ids_from_compilation_errors(errors) { - if !last_fail_module_ids.contains(&id) { - last_fail_module_ids.push(id); - } + let mut new_ids = std::collections::HashSet::new(); + for id in get_module_ids_from_compilation_errors(errors) { + new_ids.insert(id); } + last_fail_module_ids.extend(new_ids); }
135-135: Follow up on the TODO comment for dynamic inputs.
A TODO suggests support forcontext.add_dynamic_input. If this functionality is high-priority, consider opening a separate tracking ticket or implementing it soon to avoid stale todos.
486-502: Entry and dynamic entry detection works but can be consolidated.
The current conditional blocks forResolveKind::EntryandResolveKind::DynamicEntryboth set the module as an entry type. Consider using a shared helper to reduce duplication if more entry kinds are introduced later.crates/compiler/src/generate/render_resource_pots.rs (2)
37-52: Reduce potential lock overhead in the cached resources loop.
Multiple calls toresources.lock()are made in quick succession. Consider batching them into a single lock scope to improve performance and reduce locking overhead.- for cached_resource in &cached_resources.resources { - resource_pot.add_resource(cached_resource.resource.name.clone()); - } - for cached_resource in cached_resources.resources { - resources.lock().push(cached_resource.resource); - ... - resources.lock().push(map); + { + let mut locked_resources = resources.lock(); + for cached_resource in &cached_resources.resources { + resource_pot.add_resource(cached_resource.resource.name.clone()); + } + for cached_resource in cached_resources.resources { + locked_resources.push(cached_resource.resource); + ... + locked_resources.push(map); + } + }
209-209: Check the resolution of this TODO.
A TODO comment suggests additional logic for augmenting the resource hash. If unimplemented, consider adding a tracking issue to complete or remove this comment.crates/compiler/src/update/mod.rs (1)
159-159: Consider removing or clarifying commented-out code.
The// self.context.set_update();line might cause confusion if left indefinitely.- // self.context.set_update(); + // If needed later, re-enable with rationale or remove entirely.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (1)
130-233: Structured resource pot snapshot logic
The newly introduced sorting and snapshot structures (GroupResourcePotsSnapshotItem,ModuleResourcePotSnapshotItem) effectively improve clarity. One minor suggestion: consider using a loop or iterator-based approach to add each module group and module item into the vectors, to avoid repeated push calls. This can make the code more concise and maintainable.- let module_group_a = module_group_graph.module_group(&group_id_a).unwrap(); - group_resource_pots.push(GroupResourcePotsSnapshotItem { - id: module_group_a.id.clone(), - resource_pots: module_group_a.resource_pots().clone(), - }); - // ... + for group_id in [&group_id_a, &group_id_b, &group_id_d, &group_id_f, &group_id_i] { + let mg = module_group_graph.module_group(group_id).unwrap(); + group_resource_pots.push(GroupResourcePotsSnapshotItem { + id: mg.id.clone(), + resource_pots: mg.resource_pots().clone(), + }); + }crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/output.js (1)
71-71: Console log usage is acceptable for debugging.
No concerns here. It appears to confirm the presence of local binding forlodash. Consider removing or converting to a more formal logger if needed in production.crates/compiler/src/build/module_cache.rs (2)
163-163: Use ofHashSet::default()forcacheable_modules.
ReplacingHashSet::new()withHashSet::default()is functionally identical but consistent with the broader style in Rust, so no particular concerns here.
374-374: TODO about virtual modules.
A placeholder comment suggests a plan to load virtual modules fromcontext.loadfirst. This is important for accuracy in watch dependencies.Would you like help drafting an implementation snippet for handling virtual modules?
crates/compiler/tests/fixtures/css/modules/global/output.js (1)
26-28: Remove the redundant empty string.
The literal"";on line 27 appears to have no effect. Cleaning it up would enhance clarity.- "";crates/compiler/src/generate/partial_bundling.rs (1)
236-254: Well-structured separation for dynamic entries.
Introducinggenerate_dynamic_entry_resource_pots()clarifies the distinction between regular and dynamic resource pot generation. This separation is a clean design choice.crates/compiler/src/lib.rs (1)
166-166: Semicolon at the end of the block.
No functional impact; matches Rust style for ending a block expression.crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (2)
25-25: Potential runtime environment check.This IIFE references
document.currentScriptand a fallback usingrequire("url"). Ensure these references don’t break in Node environments (if SSR or Node usage is ever required).
32-34: Data URL usage for CSS background.Embedding images directly in CSS via Data URL can increase bundle size. If pictures are large or used frequently, consider referencing external resources or a caching strategy.
crates/compiler/src/update/regenerate_resources/mod.rs (2)
32-32: Clarify target environment.The doc comment says this function only works for
browsertarget. If used in other environments, it may cause unexpected behavior.
210-210: Logical placement of theprocess_resource_potshook.Ensure plugins expecting processed resource pots are still invoked in the proper order. If any plugin depends on pre-processing steps, confirm it is satisfied.
crates/compiler/tests/fixtures/bundle/library/hybrid/hybrid_entry_should_reexport/output.cjs.js (3)
20-32: Confirm object property iteration coverage in_export_star.Currently, it excludes
"default"but not other special properties (like symbols). If you need to skip more keys, adjust accordingly.
33-37: Check default condition in_interop_require_default.The check
obj && obj.__esModuleis standard. Confirm all relevant bundler code uses this pattern rather than duplicating or introducing alternate checks.
46-49: Ensure correct module references.The comment
//index.js:suggests multi-file or multi-entry usage. Double-check that requiring"./farm_runtime.js"is correct for the build output structure.crates/compiler/src/update/handle_update_modules.rs (3)
9-9: More descriptive alias or usage comment.
While importingHashMap, consider adding a type alias or descriptive comment to clarify its usage (e.g., keys/values). It enhances readability when the map usage is not immediately obvious.
52-52: Clarify naming for the local variable.
Consider renamingplugin_update_modules_hook_paramsor adding an inline comment describing the parameter, to better convey its purpose (e.g., “collectedPaths” or similar).
240-252: Double-check for duplicates.
Appendinglast_fail_module_idstopathsis helpful. However, if errors can occur repeatedly, consider de-duplicating paths in case a previously failed module is reinserted into the list multiple times.fn resolve_last_failed_module_paths( mut paths: Vec<(String, UpdateType)>, last_fail_module_ids: &[ModuleId], context: &Arc<CompilationContext>, ) -> Vec<(String, UpdateType)> { paths.extend( last_fail_module_ids .iter() .map(|id| (id.resolved_path(&context.config.root), UpdateType::Updated)), ); + // optional de-dup step + paths.sort_by_key(|(p, _)| p.clone()); + paths.dedup_by(|(p1, _), (p2, _)| p1 == p2); paths }crates/compiler/src/update/finalize_module_graph.rs (1)
34-67: Efficiently freezing affected modules.
- Merging
added_modulesandupdated_modulesensures relevant modules are processed together.- The use of
farm_profile_function!is helpful for performance analysis.- The combination of
call_freeze_module_with_paramsandupdate_modules_resolved_depslooks coherent, though consider adding checks for potential leftover entries inresolved_deps_mapif a module was removed or replaced. This step can prevent stale references.crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/output.cjs.js (2)
28-49:_interop_require_wildcardimprovements.
- The function thoroughly enumerates properties, preserving getters/setters if present.
- Check for cyclical references if your usage pattern might require it.
- The approach of returning early for ES modules is standard.
55-57: Exports object organization.
Exporting__commonJs,_interop_require_default, and_interop_require_wildcardundermodule.exportsis consistent. Consider documenting them in code comments if external usage is expected.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (3)
74-74: Use consistent spelling in the comment.It should be "always render..." instead of "alway render...".
- // alway render enforce resource pots and dynamic entry resource pots + // always render enforce resource pots and dynamic entry resource pots
117-129: Handle potential missing modules inis_module_external().When
removed_modulesdoes not contain the module, the code unconditionally callsmodule_graph.module(module_id).unwrap(). A missing module in both maps will cause a panic. Consider a more graceful error handling or an early return.- let module = if let Some(module) = removed_modules.get(module_id) { - module - } else { - module_graph.module(module_id).unwrap() - }; + let module = removed_modules.get(module_id) + .or_else(|| module_graph.module(module_id)) + .unwrap_or_else(|| { + // handle error gracefully or return false + panic!("Module {module_id:?} not found in removed_modules or module_graph") + });
131-145: Avoid panicking inremove_empty_resource_pots()when resource pot is missing.Returning an error or logging a warning might be more robust than calling
panic!, especially in production builds. Consider an alternative approach that doesn’t crash the entire process.crates/compiler/src/build/finalize_module_graph.rs (1)
134-147: Order of edge updates inupdate_modules_resolved_deps().This systematically updates or adds edges. No immediate concerns. The approach is clear, and error handling is done via
unwrap(). Consider a more descriptive error strategy for production.crates/compiler/src/update/patch_module_group_graph.rs (5)
71-82: Breadth-first approach to find final affected module groups.Be mindful of large graphs. BFS here is fine, but watch out for performance if the graph is extremely large.
184-258: Combined logic inpatch_added_dynamic_import_and_dynamic_entry().This function updates the graph in multiple ways (adding new groups, patching dynamic entry groups, adding modules to existing groups). Confirm that partial updates cannot leave the graph in an unknown state if an unexpected error arises.
260-374: Complex removal logic inpatch_removed_dynamic_import_and_dynamic_entry().When edges are removed, the function tries to remove groups or patch them. The branching logic is quite involved. Consider extracting smaller private functions for clarity.
376-459: Non-dynamic children BFS may benefit from memoization.
patch_existing_added_non_dynamic_children()repeatedly traverses dependencies. If the graph is large, repeated visits might degrade performance.
573-641: Helper functions for dynamic entry groups.
get_dynamic_entry_group_ids(),patch_dynamic_entry_group_for_added_dynamic_import(), andpatch_dynamic_entry_group_for_removed_dynamic_import()collectively manage dynamic entry updating. The approach is logical, but watch for potential duplication across these three.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
Cargo.lockis excluded by!**/*.lockcrates/compiler/src/generate/snapshots/generate_resource_pot_map-2.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map-3.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map-4.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map-5.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map_external-2.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map_external-3.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map_external-4.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map_external-5.snapis excluded by!**/*.snapcrates/compiler/src/generate/snapshots/generate_resource_pot_map_external.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-2.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-3.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-4.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-5.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-6.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-7.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-8.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots-9.snapis excluded by!**/*.snapcrates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/snapshots/generate_and_diff_resource_pots.snapis excluded by!**/*.snap
📒 Files selected for processing (207)
.changeset/clever-wasps-battle.md(1 hunks).changeset/config.json(1 hunks).changeset/dull-apes-raise.md(1 hunks).changeset/sharp-pugs-lie.md(1 hunks).changeset/shiny-guests-deliver.md(1 hunks).changeset/ten-dolls-provide.md(1 hunks).github/workflows/ci.yaml(4 hunks).github/workflows/code-spell-check.yaml(1 hunks).github/workflows/release-plz.yaml(1 hunks).github/workflows/release.yaml(2 hunks).github/workflows/rust-build.yaml(9 hunks).github/workflows/rust-test.yaml(1 hunks).gitignore(1 hunks).npmrc(1 hunks).vscode/settings.json(1 hunks)CONTRIBUTING.md(1 hunks)CONTRIBUTING.zh-CN.md(1 hunks)bench/benches/example.bench.ts(0 hunks)bench/vitest.config.mts(0 hunks)biome.json(4 hunks)configs/farm-js-plugin.base.config.mjs(1 hunks)crates/bench/CHANGELOG.md(0 hunks)crates/bench/Cargo.toml(0 hunks)crates/bench/benches/compiler_bench.rs(0 hunks)crates/compiler/Cargo.toml(2 hunks)crates/compiler/src/build/finalize_module.rs(1 hunks)crates/compiler/src/build/finalize_module_graph.rs(1 hunks)crates/compiler/src/build/mod.rs(13 hunks)crates/compiler/src/build/module_cache.rs(5 hunks)crates/compiler/src/generate/finalize_resources.rs(2 hunks)crates/compiler/src/generate/mod.rs(2 hunks)crates/compiler/src/generate/partial_bundling.rs(9 hunks)crates/compiler/src/generate/render_resource_pots.rs(4 hunks)crates/compiler/src/generate/resource_cache.rs(1 hunks)crates/compiler/src/lib.rs(8 hunks)crates/compiler/src/trace_module_graph.rs(2 hunks)crates/compiler/src/update/diff_and_patch_module_graph.rs(13 hunks)crates/compiler/src/update/diff_and_patch_module_graph/test_diff_module_deps.rs(11 hunks)crates/compiler/src/update/diff_and_patch_module_graph/test_diff_module_graph.rs(4 hunks)crates/compiler/src/update/diff_and_patch_module_graph/test_patch_module_graph.rs(3 hunks)crates/compiler/src/update/finalize_module_graph.rs(1 hunks)crates/compiler/src/update/find_hmr_boundaries.rs(8 hunks)crates/compiler/src/update/handle_update_modules.rs(7 hunks)crates/compiler/src/update/mod.rs(17 hunks)crates/compiler/src/update/patch_module_group_graph.rs(3 hunks)crates/compiler/src/update/patch_module_group_graph/test_dynamic_entries.rs(1 hunks)crates/compiler/src/update/patch_module_group_graph/tests.rs(13 hunks)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs(12 hunks)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs(7 hunks)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_dynamic_entry_resource_pots.rs(1 hunks)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs(8 hunks)crates/compiler/src/update/regenerate_resources/mod.rs(6 hunks)crates/compiler/src/utils.rs(1 hunks)crates/compiler/tests/bundle.rs(5 hunks)crates/compiler/tests/common/mod.rs(4 hunks)crates/compiler/tests/css.rs(4 hunks)crates/compiler/tests/external.rs(1 hunks)crates/compiler/tests/fixtures/_internal/runtime/index.js(0 hunks)crates/compiler/tests/fixtures/_internal/runtime/src/module-system.ts(1 hunks)crates/compiler/tests/fixtures/_internal/runtime/src/modules/dynamic-import.ts(1 hunks)crates/compiler/tests/fixtures/_internal/runtime/src/modules/module-helper.ts(1 hunks)crates/compiler/tests/fixtures/_internal/runtime/src/modules/module-system-helper.ts(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/circle/config.json(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/circle/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/circle/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/circle/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/circle/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/export/config.json(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/export/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/export/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/concatenate_module/export/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/config.cjs.json(1 hunks)crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/export/reexportExternal/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/export/reexportExternal/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/export/reexportExternal/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/export/reexportExternal/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/execute/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/execute/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/execute/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/external/importNamespace/output.cjs.js(0 hunks)crates/compiler/tests/fixtures/bundle/library/external/importNamespace/output.js(0 hunks)crates/compiler/tests/fixtures/bundle/library/external/importNamespace/output.json.js(0 hunks)crates/compiler/tests/fixtures/bundle/library/global_variable/config.json(0 hunks)crates/compiler/tests/fixtures/bundle/library/global_variable/output.js(0 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/bundle2-foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/esm_export_cjs/output.cjs.js(2 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/esm_export_cjs/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/hybrid_entry_should_reexport/output.cjs.js(2 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/hybrid_entry_should_reexport/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/a.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/output.cjs.js(2 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/output.js(2 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/bar.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/cjs.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/esm.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/normal/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/hybrid/normal/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/esm/default/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/esm/named/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/esm/namespace/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/esm/star/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/circle.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/named/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/namespace/output.js(2 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/star/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/star/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/use_external_reexport/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/use_external_reexport/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/library/reexport/use_external_reexport/reexport.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/config.cjs.json(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/config.cjs.json(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/default.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/namespace.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/entry_export/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/entry_export/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/entry_export/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/bundle2_namespace.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/bundle2-foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/foo.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/output.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/bundle2.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/config.cjs.json(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/default.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/index.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/named.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/namespace.ts(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/output.js(1 hunks)crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js(1 hunks)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js(1 hunks)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js(1 hunks)crates/compiler/tests/fixtures/css/modules/global/output.js(1 hunks)crates/compiler/tests/fixtures/css/modules/local/output.js(1 hunks)crates/compiler/tests/fixtures/css/modules/normal/output.js(1 hunks)crates/compiler/tests/fixtures/css/prefixer/normal/output.js(1 hunks)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js(1 hunks)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js(1 hunks)crates/compiler/tests/fixtures/external/browser/normal/cjs/output.js(1 hunks)crates/compiler/tests/fixtures/external/browser/normal/esm/output.js(1 hunks)crates/compiler/tests/fixtures/external/browser/object/cjs/output.js(1 hunks)crates/compiler/tests/fixtures/external/browser/object/esm/output.js(1 hunks)crates/compiler/tests/fixtures/external/node/hosting/import/output.js(1 hunks)crates/compiler/tests/fixtures/external/node/hosting/require/output.js(1 hunks)crates/compiler/tests/fixtures/external/node/normala/cjs/output.js(1 hunks)crates/compiler/tests/fixtures/external/node/normala/esm/output.js(1 hunks)crates/compiler/tests/fixtures/library/external/import-named/index.ts(1 hunks)crates/compiler/tests/fixtures/library/external/import-named/output.js(1 hunks)crates/compiler/tests/fixtures/library/external/import-namespace/config.cjs.json(1 hunks)crates/compiler/tests/fixtures/library/external/import-namespace/config.json(1 hunks)crates/compiler/tests/fixtures/library/external/import-namespace/output.cjs.js(1 hunks)crates/compiler/tests/fixtures/library/external/import-namespace/output.js(1 hunks)crates/compiler/tests/fixtures/library/ident-conflicts/global-variable/config.json(1 hunks)crates/compiler/tests/fixtures/library/ident-conflicts/global-variable/output.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/a.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/b.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/c.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/d.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/e.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/f.js(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/index.ts(1 hunks)crates/compiler/tests/fixtures/library/reexport/basic/output.js(1 hunks)crates/compiler/tests/fixtures/library/runtime/index.ts(1 hunks)crates/compiler/tests/fixtures/library/runtime/module-system.ts(1 hunks)crates/compiler/tests/fixtures/library/runtime/modules/module-helper.ts(1 hunks)crates/compiler/tests/fixtures/library/runtime/modules/plugin.ts(1 hunks)crates/compiler/tests/fixtures/library/runtime/output.js(1 hunks)crates/compiler/tests/fixtures/partial_bundling/basic/output.js(1 hunks)crates/compiler/tests/fixtures/runtime/bundle/browser/external/output.js(1 hunks)crates/compiler/tests/fixtures/runtime/bundle/browser/normal/output.js(1 hunks)crates/compiler/tests/fixtures/runtime/bundle/cjs/export/circlesImportCjs/output.js(1 hunks)crates/compiler/tests/fixtures/runtime/bundle/cjs/export/entryExportStar/output.js(2 hunks)crates/compiler/tests/fixtures/runtime/bundle/cjs/export/exportNamed/output.js(1 hunks)crates/compiler/tests/fixtures/runtime/bundle/cjs/export/exportNamespace/output.js(2 hunks)crates/compiler/tests/fixtures/runtime/bundle/cjs/export/exportStar/output.js(1 hunks)
⛔ Files not processed due to max files limit (66)
- crates/compiler/tests/fixtures/runtime/bundle/cjs/export/repeatUsingCjs/output.js
- crates/compiler/tests/fixtures/runtime/bundle/cjs/hybrid/exportStar/output.js
- crates/compiler/tests/fixtures/runtime/bundle/cjs/hybrid/nested_require/output.js
- crates/compiler/tests/fixtures/runtime/bundle/cjs/hybrid/unresolved_variables/output.js
- crates/compiler/tests/fixtures/runtime/bundle/cjs/normal/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/export_default_decl/no_ident/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/forward_export/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/normal/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/self_ref/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/spread/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/unresolved_variable/output.js
- crates/compiler/tests/fixtures/runtime/bundle/decl/var/output.js
- crates/compiler/tests/fixtures/runtime/bundle/dynamic/normal/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/emptyExport/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/entryExport1/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/entryExport2/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/entryExport3/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/exportDefault1/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/exportDefaultDecl/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/exportDefaultExpr/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/exportNamed1/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/exportNamed2/output.js
- crates/compiler/tests/fixtures/runtime/bundle/export/exportNamed3/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/export/from_source/export_all/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/export/from_source/export_as_namespace/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/export/from_source/export_named/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/export/from_source/export_namespace/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/import/default/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/import/named/output.js
- crates/compiler/tests/fixtures/runtime/bundle/external/import/namespace/output.js
- crates/compiler/tests/fixtures/runtime/bundle/function/output.js
- crates/compiler/tests/fixtures/runtime/bundle/import/importDefault/output.js
- crates/compiler/tests/fixtures/runtime/bundle/import/importNamed/output.js
- crates/compiler/tests/fixtures/runtime/bundle/import/importNamespace/output.js
- crates/compiler/tests/fixtures/runtime/bundle/import/importedInScope/output.js
- crates/compiler/tests/fixtures/runtime/bundle/import/sameImport/output.js
- crates/compiler/tests/fixtures/runtime/bundle/interface/output.js
- crates/compiler/tests/fixtures/runtime/bundle/issues/merge_props/output.js
- crates/compiler/tests/fixtures/runtime/bundle/lazy_compilation/output.js
- crates/compiler/tests/fixtures/runtime/bundle/polyfill/reserved_word/cjs/output.js
- crates/compiler/tests/fixtures/runtime/bundle/polyfill/reserved_word/esm/output.js
- crates/compiler/tests/fixtures/runtime/bundle/rename/output.js
- crates/compiler/tests/fixtures/script/accept_deps/output.js
- crates/compiler/tests/fixtures/script/comments/output.js
- crates/compiler/tests/fixtures/script/import_equals/output.js
- crates/compiler/tests/fixtures/script/import_meta/url/import_meta_url_with_URL/absolute_public_path/lit_str/output.js
- crates/compiler/tests/fixtures/script/import_meta/url/import_meta_url_with_URL/relative_public_path/tip/complate/output.js
- crates/compiler/tests/fixtures/script/import_meta/url/import_meta_url_with_URL/relative_public_path/tip/normal/output.js
- crates/compiler/tests/fixtures/script/import_meta/url/import_meta_url_with_URL/relative_public_path/tip/tip_star_star/output.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.browser-cjs.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.browser-esm.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.library-browser-cjs.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.library-browser-esm.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.library-node-cjs.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.library-node-esm.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.node-cjs.js
- crates/compiler/tests/fixtures/script/import_meta/url/node/output.node-esm.js
- crates/compiler/tests/fixtures/script/native_top_level_await/output.js
- crates/compiler/tests/fixtures/tree_shake/basic/output.js
- crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js
- crates/compiler/tests/fixtures/tree_shake/cjs/output.js
- crates/compiler/tests/fixtures/tree_shake/compiled-vue-comp/output.js
- crates/compiler/tests/fixtures/tree_shake/css/output.js
- crates/compiler/tests/fixtures/tree_shake/cyclic_deps/output.js
- crates/compiler/tests/fixtures/tree_shake/cyclic_export_all/output.js
💤 Files with no reviewable changes (11)
- crates/compiler/tests/fixtures/_internal/runtime/index.js
- crates/bench/CHANGELOG.md
- crates/compiler/tests/fixtures/bundle/library/global_variable/config.json
- crates/bench/Cargo.toml
- bench/vitest.config.mts
- bench/benches/example.bench.ts
- crates/bench/benches/compiler_bench.rs
- crates/compiler/tests/fixtures/bundle/library/external/importNamespace/output.cjs.js
- crates/compiler/tests/fixtures/bundle/library/external/importNamespace/output.json.js
- crates/compiler/tests/fixtures/bundle/library/external/importNamespace/output.js
- crates/compiler/tests/fixtures/bundle/library/global_variable/output.js
✅ Files skipped from review due to trivial changes (73)
- .changeset/ten-dolls-provide.md
- crates/compiler/tests/fixtures/bundle/library/execute/bundle2.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/default.ts
- crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/config.cjs.json
- crates/compiler/tests/fixtures/bundle/library/execute/index.ts
- crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/circle.ts
- crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/foo.ts
- crates/compiler/tests/fixtures/_internal/runtime/src/module-system.ts
- crates/compiler/tests/fixtures/library/external/import-namespace/config.cjs.json
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/config.cjs.json
- crates/compiler/tests/fixtures/bundle/concatenate_module/circle/index.ts
- crates/compiler/tests/fixtures/library/ident-conflicts/global-variable/config.json
- crates/compiler/tests/fixtures/library/reexport/basic/e.js
- crates/compiler/tests/fixtures/library/reexport/basic/c.js
- crates/compiler/tests/fixtures/bundle/concatenate_module/export/reexport.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/config.cjs.json
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/default.ts
- crates/compiler/tests/fixtures/bundle/concatenate_module/export/config.json
- crates/compiler/tests/fixtures/library/external/import-namespace/config.json
- crates/compiler/tests/fixtures/bundle/export/reexportExternal/foo.ts
- crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/index.ts
- crates/compiler/tests/fixtures/bundle/export/reexportExternal/index.ts
- crates/compiler/tests/fixtures/library/reexport/basic/d.js
- crates/compiler/tests/fixtures/bundle/concatenate_module/export/index.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/config.cjs.json
- crates/compiler/tests/fixtures/bundle/concatenate_module/circle/config.json
- .gitignore
- crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/index.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/index.ts
- crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/a.ts
- .github/workflows/code-spell-check.yaml
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/bundle2.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/bundle2-foo.ts
- crates/compiler/tests/fixtures/bundle/concatenate_module/circle/reexport.ts
- crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/bundle2-foo.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/entry_export/index.ts
- crates/compiler/tests/fixtures/library/ident-conflicts/global-variable/output.js
- crates/compiler/tests/fixtures/library/reexport/basic/f.js
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/bundle2.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/entry_export/bundle2.ts
- crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/bar.ts
- crates/compiler/tests/fixtures/bundle/export/reexportExternal/reexport.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/bundle2_namespace.ts
- .changeset/clever-wasps-battle.md
- crates/compiler/tests/fixtures/library/runtime/module-system.ts
- crates/compiler/tests/fixtures/bundle/library/reexport/reexport/index.ts
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/index.ts
- crates/compiler/tests/external.rs
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/index.ts
- crates/compiler/tests/fixtures/library/external/import-namespace/output.js
- crates/compiler/tests/fixtures/library/reexport/basic/b.js
- crates/compiler/tests/fixtures/library/runtime/index.ts
- crates/compiler/tests/fixtures/bundle/export/reexportExternal/output.js
- crates/compiler/tests/fixtures/library/reexport/basic/a.js
- crates/compiler/tests/fixtures/library/external/import-named/output.js
- crates/compiler/tests/fixtures/bundle/library/reexport/use_external_reexport/index.ts
- crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/foo.ts
- crates/compiler/tests/fixtures/library/external/import-namespace/output.cjs.js
- crates/compiler/tests/fixtures/bundle/library/reexport/reexport/reexport.ts
- crates/compiler/tests/fixtures/library/runtime/output.js
- crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/namespace.ts
- .github/workflows/release-plz.yaml
- crates/compiler/tests/fixtures/bundle/library/execute/output.js
- crates/compiler/tests/fixtures/bundle/library/reexport/reexport/output.js
- configs/farm-js-plugin.base.config.mjs
- crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/output.js
- .changeset/sharp-pugs-lie.md
- crates/compiler/tests/fixtures/bundle/library/reexport/use_external_reexport/output.js
- crates/compiler/tests/fixtures/bundle/library/reexport/use_external_reexport/reexport.ts
- crates/compiler/src/trace_module_graph.rs
- .changeset/shiny-guests-deliver.md
- crates/compiler/tests/fixtures/library/external/import-named/index.ts
- crates/compiler/src/update/diff_and_patch_module_graph/test_diff_module_graph.rs
🔇 Additional comments (382)
crates/compiler/tests/fixtures/bundle/library/reexport/esm/star/output.js (2)
1-3: LGTM: Runtime header format is clear and maintainable.The bundler runtime header clearly identifies the module ID slot for polyfills.
6-7: LGTM: Module identification is well-structured.The module ID comment makes it clear which source file this code represents, and the variable declaration is correctly formatted.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/namespace/output.js (3)
1-13: The bundled module export pattern looks correct.This section of the file correctly implements a namespace export pattern, creating a namespace object with proper properties and the
__esModule: trueflag for module interoperability. The bundle structure properly separates the source module (bundle2_namespace.ts) from the re-exporting module (bundle2.ts).
15-17: Empty runtime polyfill placeholder is expected.The Farm runtime polyfill slot is currently empty, which appears intentional for this test fixture.
19-24: Import and usage pattern demonstrates proper namespace handling.The import statement correctly references the exported namespace from bundle1.js, and the console.log demonstrates how consumers would access the namespace. This validates that the bundling process properly maintains the namespace structure across bundle boundaries.
crates/compiler/tests/fixtures/runtime/bundle/browser/external/output.js (1)
2-2: Double semicolon syntax added to generated outputThe change adds an extra semicolon before the immediately invoked function expression. While this doesn't affect functionality (multiple semicolons are treated as empty statements in JavaScript), it represents a deliberate change in the generated output format.
This syntax change is likely part of the broader Farm v2.0 refactoring mentioned in the PR objectives. The double semicolon pattern is sometimes used in bundlers to ensure proper statement separation or to mark boundaries between different sections of bundled code.
crates/compiler/tests/fixtures/runtime/bundle/cjs/export/circlesImportCjs/output.js (3)
2-18: Well-implemented module interoperability function.The new
__commonJsfunction elegantly handles module initialization with proper memoization. It correctly addresses circular dependencies by lazily initializing modules and returning cached results when available. The implementation supports both function-style modules and object modules with a reasonable fallback mechanism.
19-19: Clean usage of the new CommonJS wrapper.The implementation properly uses the new
__commonJsfunction to wrap the module, which is consistent with the broader refactoring across the codebase to simplify module handling.
28-28: Circular dependency handled appropriately.The circular dependency between
cjs_ts_cjsandexport_ts_cjsis properly handled through the lazy initialization pattern in the__commonJsfunction. This demonstrates that the new implementation correctly addresses complex module relationships.crates/compiler/tests/fixtures/_internal/runtime/src/modules/module-system-helper.ts (1)
1-3: Simple initialization function looks good.This function appears to be part of a testing fixture for the module system, simply logging its filename to the console when called.
crates/compiler/tests/fixtures/_internal/runtime/src/modules/module-helper.ts (1)
1-3: Simple initialization function looks good.This function logs its module name to the console when called, similar to other module initialization functions in the test fixtures.
crates/compiler/tests/fixtures/_internal/runtime/src/modules/dynamic-import.ts (1)
1-3: Simple initialization function looks good.This function follows the same pattern as other module initialization functions in the test fixtures.
crates/compiler/tests/fixtures/library/runtime/modules/module-helper.ts (1)
1-5: Function adds a method to the module system object.Unlike the other initialization functions in the test fixtures, this one takes a parameter
msand adds a method_mto it that logs 'module-system' when called. Correct functionality assuming the module system object is passed as intended.crates/compiler/tests/fixtures/library/runtime/modules/plugin.ts (1)
3-7: LGTM! Clean implementation of module system plugin initialization.The
initModuleSystemfunction provides a clear mechanism for adding plugin functionality to the module system. This aligns well with the broader module system refactoring in Farm v2.0. The implementation is simple and focused, which is appropriate for a test fixture.crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/output.js (3)
1-13: The CommonJS module export pattern looks good.The implementation correctly defines a minimal lodash-like module with a merge method, and exports it as both the default export and with a named property. The usage of
__commonJswrapper is appropriate for CommonJS modules in an ES module environment.
17-33: CommonJS module loader implementation is well structured.The
__commonJsfunction provides proper module caching and initialization, ensuring modules are only initialized once. It handles both function-style modules and object modules with named exports correctly.
34-38: Interop utility follows standard patterns.The
_interop_require_defaultfunction correctly implements the standard pattern for harmonizing CommonJS and ES module exports, ensuring consistent access to default exports regardless of the module system.crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/entry_export/output.js (3)
1-6: Bundle structure appears correct for testing re-exportsThe bundle1.js segment correctly defines and exports both a named export (
named) and a default export (bundle2_default) from the original bundle2.ts module. This demonstrates the bundler's ability to preserve export structures.
8-10: Placeholder for runtime polyfills is properly maintainedThe
__FARM_BUNDLE_POLYFILL_SLOT__is correctly included as a placeholder where Farm bundler would inject necessary runtime polyfills.
12-15: Re-export implementation looks correctThe index.js correctly imports from the bundled output file and re-exports both the default and named exports. The aliasing of
bundle2_defaulttodefaultfollows standard ES module patterns for re-exporting default exports..changeset/dull-apes-raise.md (1)
1-3: Review YAML Frontmatter Structure for Version BumpThe YAML frontmatter correctly indicates the patch update for
@farmfe/plugin-dts. Please confirm that a patch bump is intended here and aligns with your release strategy.crates/compiler/tests/fixtures/bundle/library/reexport/esm/named/output.js (3)
1-3: Added bundle polyfill slot markerThe code now includes explicit module ID markers, which helps with module identification and debugging. This is part of a broader change to improve how modules are labeled in the bundle output.
6-7: Module ID and variable definition added for foo.tsClear module identification with the proper module ID helps with traceability in the bundled output. This change maintains the core functionality while making the bundled code structure more transparent.
10-11: Added module ID marker for index.tsThe export statement is now clearly associated with its source module through the added module ID comment. This improves code readability and maintainability by making the module boundaries explicit.
crates/compiler/tests/fixtures/bundle/library/reexport/esm/default/output.js (3)
1-3: Added bundle polyfill slot markerThe code now includes explicit module ID markers for the Farm runtime, which improves module identification in the bundled output.
6-7: Module ID and variable definition added for foo.tsThe module structure is now more explicit with the added module ID comment. The default export value 'foo' is maintained, showing the simplified approach to handling default exports without complex interop helpers.
9-10: Added module ID marker for index.ts with default exportThe default export is now handled in a more straightforward manner, with the module ID clearly identifying the source. This simplification likely improves performance and readability.
crates/compiler/tests/fixtures/bundle/library/reexport/esm/namespace/output.js (4)
1-3: Added bundle polyfill slot markerThe code now includes explicit module ID markers for the Farm runtime, providing better module identification in the bundled output.
6-8: Module ID and variable definitions added for foo.tsThe module structure is now clearer with the added module ID comment. Both the default export (
foo_default) and the named export (foo) are defined here, preparing for namespace export handling.
12-12: Updated namespace object property referenceThe namespace object now correctly references the
foovariable from line 8, maintaining proper namespace exports structure.
17-19: Added module ID marker and updated variable namingThe module structure is now more explicit with:
- Clear module ID for index.ts
- Variable
foo$1now holds the value 123- Console log statement references both the namespace exports and the local variable
This change maintains functionality while making the module structure more transparent.
crates/compiler/tests/fixtures/bundle/library/hybrid/esm_export_cjs/output.cjs.js (2)
33-37: Proper export of utility functions with ESModule marking.The code correctly exports the utility functions and marks the module as an ES module, which is essential for proper interoperability.
42-52: Clean implementation of CommonJS module using the new utility.The code correctly imports and uses the utility functions to create a CommonJS module with proper exports.
crates/compiler/tests/fixtures/bundle/library/hybrid/normal/output.cjs.js (4)
25-29: Properly exporting runtime utilitiesGood approach to export the utility functions and set the
__esModuleflag, ensuring compatibility between CommonJS and ES modules.
34-37: Module imports and interop handlingEffective use of the runtime utilities to handle module imports, particularly for Node.js built-in modules.
44-49: Streamlined property export functionThe
_exporthelper now uses theallparameter directly for defining property getters, which is more straightforward than previous approaches.
50-60: Well-structured exports with named and default exportsThe code properly defines both named exports (
foo,bar) and a default export, following best practices for hybrid CommonJS/ES modules compatibility.Also applies to: 67-68
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/bundle2.ts (1)
1-7: Test fixture looks good.This module correctly imports from './foo', includes a side-effect import of './bundle2-foo', and exports two string constants. The structure appears appropriate for testing cross-bundle re-exports.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/foo.ts (1)
1-11: Test fixture is properly structured.The module exports two named constants and a default export while keeping some variables internal. The logging statement helps verify module execution during bundling tests.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/index.ts (1)
1-14: Test fixture is well designed for cross-bundle testing.This module correctly imports from './bundle2' and defines its own exports. The intentional shadowing of variable names (
foo_str1andfoo_str3) with different types/values than in 'foo.ts' appears to be part of testing variable scope resolution in the bundler.The commented line for re-exporting from './bundle2' provides an option for additional test scenarios.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/namespace.ts (1)
1-4: Mixed module system - intentional but worth notingThis file combines ESM (line 1-2) and CommonJS (line 4) export styles in the same module. While this is valid and works with bundlers supporting interoperability, it's generally considered a non-standard practice that could lead to unexpected behavior in some environments.
Since this appears to be a test fixture for evaluating the bundler's module interop capabilities, this mixing is likely intentional to verify proper handling of hybrid modules.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/output.js (3)
11-27: Well-implemented CommonJS module wrapperThe
__commonJsfunction implementation is robust with:
- Caching mechanism to avoid re-executing modules
- Proper handling of both function and object-based modules
- Correct module initialization with empty exports object
This follows best practices for implementing CommonJS compatibility in an ESM environment.
28-57: Thorough namespace import interoperability implementationThe
_interop_require_wildcardfunction properly handles:
- Caching with WeakMap for performance optimization
- Correct handling of __esModule flag
- Property descriptors preservation (getters/setters)
- Default export assignment
- Non-enumerable properties via
__proto__: nullThis implementation ensures proper compatibility between different module systems.
65-89: Correct handling of mixed export stylesThe bundler correctly processes the mixed export styles from the source file:
- Named exports (
ns_named,ns_default) are properly handled via the_exporthelper- CommonJS-style export (
module.exports.name) is preserved in the module wrapperThis demonstrates proper interoperability between ESM and CommonJS within the same module.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/cross_bundle/namespace/output.cjs.js (3)
1-13: Proper CommonJS output transformationThe ESM imports are correctly transformed to CommonJS
require()calls, and exports are properly set usingmodule.exports. The__esModuleflag is correctly set to maintain compatibility with ESM imports of this CommonJS module.
77-101: Mixed module handling in CommonJS formatThe bundler correctly handles the mixed module format from the source file:
- The ESM named exports are properly converted to getters via the
_exporthelper- The CommonJS-style export (
module.exports.name) is preserved as-isThis demonstrates proper handling of hybrid module syntax in a CommonJS output format.
107-118: Comprehensive module exportsThe final exports include both the original exports and the processed/transformed values. The
__esModuleflag is set correctly to ensure proper interoperability when this CommonJS module is imported by an ESM module.crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/bundle2.ts (5)
1-2: TypeScript check disabling is appropriate for test purposes.Using
// @ts-nocheckis suitable for test fixtures where type checking might be less essential or even counterproductive.
3-5: Simple lodash function declaration properly mimics real library structure.The empty function declaration serves as an appropriate namespace container for the test fixture, matching the pattern used by the actual lodash library.
5-5: Method attachment pattern correctly simulates lodash API.Attaching the
mergemethod to thelodashfunction follows the pattern used by the actual lodash library, making this a good representation for testing purposes.
7-7: Aliasing with underscore mirrors lodash convention.Creating the
_alias mirrors how lodash is typically imported and used in projects, making this test fixture more realistic.
9-9: Dual export pattern suitable for testing CommonJS compatibility.The expression
(module.exports = _)._ = _simultaneously:
- Sets the default export to the lodash function
- Creates a named export accessible via the
_propertyThis pattern is typical in CommonJS modules that need to support multiple import styles and aligns with the module export interop improvements mentioned in the PR.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/named.ts (1)
1-1: Well-structured export statement.The code follows TypeScript best practices for exporting a constant.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/bundle2.ts (1)
1-7: Demonstrates good usage of various import/export patterns.This file correctly demonstrates multiple import/export patterns:
- Default import from './default'
- Namespace import from './namespace'
- Named import with alias from './named'
- Re-exporting all imported entities
The code follows TypeScript best practices and helps validate the bundling functionality.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/output.js (1)
1-37: Generated ES module bundle output looks correct.This file appears to be an automatically generated output from the Farm bundler using ES module syntax. It correctly:
- Maintains module boundaries with appropriate comments
- Preserves imports and exports from the source modules
- Creates namespace objects properly
- Handles default and named exports correctly
The bundle structure follows expected ES module output conventions.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle_1/output.cjs.js (1)
1-50: Generated CommonJS bundle output looks correct.This file demonstrates the CommonJS output format of the Farm bundler. It correctly:
- Converts ES module syntax to CommonJS
- Uses
require()for imports andmodule.exportsfor exports- Adds the
__esModuleflag for interoperability- Preserves module structure from the source files
One minor note is the use of
exportson lines 13-15 without prior definition, though this is typically available in CommonJS environments.crates/compiler/tests/fixtures/library/reexport/basic/index.ts (2)
1-22: Well-documented expected outputsThe doc comment provides clear expectations for the module's output, which is helpful for testing and understanding the re-export patterns. This documentation matches the implementation and serves as a good reference for the test case.
23-26: LGTM - Clear import and logging patternThe imports and console logging are appropriate for demonstrating the behavior of the namespaced imports.
crates/compiler/tests/fixtures/library/reexport/basic/output.js (2)
3-25: Namespace object implementation with gettersThe implementation of namespace objects using getters is a good approach for lazy evaluation of re-exported modules. This pattern ensures that circular dependencies are properly handled and values are evaluated only when accessed.
Also applies to: 43-65
66-72: Export implementation matches documented expectationsThe exports align with the expected output described in the index.ts documentation comments. The pattern of exporting namespace objects and individual named exports properly captures the re-export behavior you're testing.
crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/output.js (6)
1-6: Proper module encapsulation.The bundled output correctly isolates the
bundle2-foo.tsmodule into its own namespace with the exportedfoo$5object.
15-16: Correct import handling.The bundler has properly resolved the import from the external bundle and maintains the expected module structure.
19-21: Empty return statement preserves original code structure.The function correctly maintains the empty return statement from the source file.
22-26: Arrow function bundling works as expected.The bundler has correctly preserved the nested arrow function structure while properly referencing the imported variable.
27-35: Class definition correctly preserved.The bundler has maintained the class structure, including constructor with default parameter and method definition.
39-44: Computed property names handled correctly.The bundled output properly preserves the computed property names using variables, which can be challenging for some bundlers.
crates/compiler/tests/fixtures/bundle/library/hybrid/defaultParams/index.ts (2)
9-13: Function returns another function with proper type annotation.The arrow function implementation correctly uses TypeScript type annotation for the parameter.
15-21: Class uses TypeScript parameter property shorthand.The class effectively uses TypeScript's parameter property shorthand to declare and initialize the class property in one line.
crates/compiler/tests/fixtures/bundle/concatenate_module/circle/foo.ts (1)
1-4: Clean implementation of a simple TypeScript module.The module correctly imports the
reexportvalue from a relative path and exports a new value that concatenates it with a string literal. The code structure is clear and follows good module design practices.crates/compiler/tests/fixtures/bundle/concatenate_module/export/output.js (3)
3-19: Well-structured CommonJS compatibility function.The
__commonJsimplementation is clean and handles both function-based and object-based module definitions appropriately. It implements proper module caching with the closure pattern.
20-27: Runtime initialization looks good.The code properly initializes the runtime module using the
__commonJsfunction and immediately invokes it. The module registers empty plugins with the module system.
28-33:❓ Verification inconclusive
Verify module implementation logic.
The module registration and initialization pattern looks correct, but ensure that the
farmRequire("xxx")call will resolve correctly in this test environment, as "xxx" appears to be an external dependency that may need to be properly mocked.
🏁 Script executed:
#!/bin/bash # Check if there's a module named "xxx" defined somewhere in the test fixtures fd -t f -e js -e ts -e json "xxx" crates/compiler/tests/fixtures/Length of output: 65
Attention: External Dependency 'xxx' Verification
The module registration and initialization logic in the output file appears correct. However, our search did not locate any definition or mock for the dependency
"xxx"in the test fixtures. Please ensure that the test environment provides a proper mock or stub for"xxx"so that thefarmRequire("xxx")call resolves as expected.
- Action Required: Confirm that there is a mock for the external dependency
"xxx"when running tests or update the fixture to include it if necessary.crates/compiler/tests/fixtures/bundle/concatenate_module/circle/output.js (3)
3-19: Well-implemented module loading mechanism.The
__commonJsfunction provides an effective way to handle CommonJS-style modules with proper caching and execution.
28-52: Circular dependency correctly handled.The code implements modules with circular dependencies (
foo.tsandreexport.tsimport each other). This pattern correctly uses getters viamodule.o()to delay value resolution, avoiding initialization problems with circular references.
44-52:❓ Verification inconclusive
Verify the re-export mechanism.
The
module._e(exports, _f_foo)call in thereexport.tsmodule is re-exporting from thefoo.tsmodule, which creates a circular dependency. While the code handles this correctly with getter functions, ensure this specific test case is validating that circular references work as expected.
🏁 Script executed:
#!/bin/bash # Check if there's a test case that verifies circular module dependencies grep -r "circle" --include="*.rs" crates/compiler/tests/Length of output: 56
Verify that the circular dependency test is correctly validating re-export behavior.
The re-export in
reexport.ts(in which the call tomodule._e(exports, _f_foo)re-exports fromfoo.ts) is intended to trigger circular dependency handling via getters. The test fixture (crates/compiler/tests/fixtures/bundle/concatenate_module/circle/output.js) appears designed for this, but our automated search (limited to Rust files) did not locate any references to "circle." Since the test is implemented in JavaScript rather than Rust, please manually verify that this fixture properly exercises circular dependency scenarios.crates/compiler/tests/bundle.rs (6)
17-17: Function signature change looks good.The addition of an optional closure parameter allows for more flexible configuration during test execution, which is a good pattern for test customization in Rust.
36-36: Note the mode change from Production to Development.The configuration mode has been changed from Production to Development. This may impact how the code is optimized or bundled during tests.
Consider whether both modes should be tested to ensure complete coverage, as they can behave differently.
61-63: Good implementation of the optional configuration closure.The conditional application of the optional closure is implemented correctly and provides a clean way to customize configurations for different tests.
76-77: Configuration update for assertion checks.The assertion configuration has been updated to not ignore the emitted field, which may catch more issues during testing.
87-98: Good implementation of multiple_bundle_test.This function provides a specialized configuration for testing multiple bundles, which is a key enhancement in this change.
102-108: Review commented-out test cases.Several specific test cases are commented out in the testing macro.
Consider whether these commented-out test cases should be enabled to ensure comprehensive test coverage, especially if they test important edge cases or features.
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/output.js (3)
1-42: Test fixture demonstrates cross-bundle reexport functionalityThis appears to be a test fixture validating the bundler's ability to handle re-exports across multiple bundles. It successfully demonstrates:
- Variable exports and re-exports
- Default exports
- Renamed exports
- Cross-bundle imports
The structure is consistent with what would be expected for a bundler test case, and it appears to provide good coverage of the functionality.
3-6:❌ Incorrect review comment
Circular dependency detected between modules
There appears to be a circular dependency between bundle1.js and index.js:
- bundle1.js imports from "./index.js" (line 3)
- index.js imports from "./bundle1.js" (line 24)
Circular dependencies can lead to unexpected behavior, especially with values that are used before they're fully initialized.
Also applies to: 24-31
🏁 Script executed:
#!/bin/bash # Check if this is an intentional test case for circular dependencies # Look for test descriptions or comments in the fixture directory fd -t f -e md -e txt . crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle cat crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/*.{md,txt} 2>/dev/null || echo "No description files found" # Look at the source files that generate this output fd -t f -e js -e ts . crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle | grep -v output.jsLength of output: 874
Clarify Circular Dependency Reference
The review comment indicates a circular dependency between
bundle1.jsandindex.js, but our inspection of the fixture shows that no file namedbundle1.jsexists in this test case. The files present in the directory are:
index.tsfoo.tsbundle2.tsbundle2-foo.tsoutput.jsIt appears the reference to
bundle1.jsis either outdated or incorrect. Please verify whether the circular dependency is meant to involve a different module (for example, one of the listed.tsfiles) or if the comment should be removed altogether.Likely an incorrect or invalid review comment.
18-20:❓ Verification inconclusive
Empty runtime section
The farm_runtime.js section is empty except for a module ID comment. According to the AI summary, this section should contain a
__commonJsfunction for module loading and exports, but it's missing from the actual code.This might indicate that the runtime utilities aren't being correctly included in the bundled output.
🏁 Script executed:
#!/bin/bash # Check if this is expected or if other test fixtures have runtime utilities fd -p "farm_runtime.js" -t f . crates/compiler/tests/fixtures/bundle | grep -v "reexport_cross_bundle" | head -n 5Length of output: 112
Action: Verify Runtime Utilities Inclusion in Reexport Bundle
The current output file for the reexport bundle (at
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/output.js, lines 18–20) shows an empty runtime section—only a comment (// module_id: __FARM_BUNDLE_POLYFILL_SLOT__) is present. Our initial check for other instances of a non-emptyfarm_runtime.jsacross the bundle fixtures (excluding reexport_cross_bundle) produced no output. This makes it unclear whether the absence of the__commonJsmodule loader (as expected by earlier summaries) is a bug or an intentional change in behavior for this test fixture.
- File under review:
crates/compiler/tests/fixtures/bundle/multiple_bundle/reexport/reexport_cross_bundle/output.js(lines 18–20)- Concern: The runtime section is empty instead of including a
__commonJsfunction for module loading.At this point, the repository-wide search did not reveal any alternative runtime utility (e.g., a non-empty
farm_runtime.js) in other fixtures. Please manually verify if the runtime utils are supposed to be injected for reexport bundles or if the current behavior is correct.crates/compiler/tests/fixtures/external/browser/object/cjs/output.js (1)
19-19: Leverage unit tests to ensure__commonJsusage is correct
The call to__commonJs((module, exports)=>{...})seems consistent with the function design. Validating this usage via unit or integration tests will help confirm that module exports/caching behave as expected.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/star/output.js (2)
20-25: Exporting_interop_require_defaulttogether with__commonJslooks fine.These exports seem consistent with a custom bundling approach for CommonJS and ES modules interop. Ensure that downstream references correctly rely on
_interop_require_defaultfor handling ES modules.
60-63: Re-export statements look straightforward.The final re-export pattern using
{ bar, foo, foo_default as default }is clear. No obvious issues.crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/foo.ts (1)
1-1: The re-export looks good.This syntax correctly re-exports the
Workerfrom the./reexportmodule, maintaining the appropriate module structure.crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/index.ts (3)
2-3: The imports look good.Correctly importing
readFiledirectly from './reexport' andWorkerfrom './foo' (which re-exports it from './reexport').
5-5: Console logging is appropriate for testing purposes.This statement logs both imported entities, which helps verify that the imports are working correctly.
7-7: The re-export syntax is correct.This re-exports all named exports from './reexport', providing a clean way to access these exports through this module.
crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/reexport.ts (2)
1-2: Re-exporting Node.js built-in modules looks good.Correctly using the
export * fromsyntax to re-export all named exports from the Node.js built-in modules.
4-5:❌ Incorrect review comment
Mixing ES modules and CommonJS syntax.
This file mixes ES modules syntax (
export * from) with CommonJS syntax (module.exports), which can lead to unexpected behavior in certain environments. Additionally,readFileis defined but not exported with ES modules syntax.
🏁 Script executed:
#!/bin/bash # Check if this mixed module pattern is used elsewhere in the codebase rg --type ts "export \* from .+;\s+.+module\.exports\." -g "!**/node_modules/**" -g "!**/dist/**"Length of output: 99
Clarification on Module Export Style
After verifying the file, it appears that the file now consistently uses CommonJS syntax (i.e. using
module.exports) without any ES module export (such asexport * from …). There isn’t any evidence of mixed export types in the current code. If the intended design is to use CommonJS exclusively, then the code is correct as is. On a side note, the constantreadFileis defined but not exported—please confirm whether it should be exported or removed.Likely an incorrect or invalid review comment.
crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/output.cjs.js (3)
3-62: Runtime helper functions look well-implemented.The CommonJS interoperability functions are properly implemented:
__commonJscorrectly handles module initialization and caching_getRequireWildcardCachemanages cache for interop requirements_interop_require_wildcardcorrectly handles wildcard imports with proper fallbacks_export_starproperly transfers properties between modulesThese functions follow standard patterns for managing module interoperability.
75-86: CommonJS module implementation looks correct.The implementation of the reexport module using the CommonJS pattern is correct, re-exporting from Node.js modules and setting the appropriate properties.
91-96: Index module implementation is correct.The bundled implementation of the index module correctly logs the imported entities and re-exports all exports from the reexport module.
crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/output.js (5)
3-19: CommonJS module wrapper implementation looks solid.The
__commonJsfunction correctly implements module caching and handles both function and object-based module definitions. It follows the CommonJS pattern where modules are executed only once and their exports are cached.
20-24: Node.js require function implementation looks correct.The implementation correctly creates a Node.js require function using
createRequirefrom the module package, which is the recommended approach for ES modules to access CommonJS modules.
52-53: Redundant exports.These lines re-export from node modules that are already re-exported through the CommonJS module (lines 37-38). While this might be intentional for the double re-export test, it creates duplicate exports.
This is likely intentional to test the "doubleReexportAll" functionality as indicated by the fixture name.
37-38:✅ Verification successful
Missing _export_star function implementation.
The code calls
_export_starfunction, but its implementation is not provided in this file. This could cause runtime errors.
🏁 Script executed:
#!/bin/bash # Search for _export_star implementation in the codebase rg "_export_star" -A 5 -B 5Length of output: 46051
Verified: The
_export_starfunction is provided via an external helper.Upon inspection, the implementation of
_export_starisn’t included inline in the ESM output file (output.js) because it’s intentionally supplied by the bundler’s runtime. The grep results show that the helper is implemented in the corresponding CommonJS file (output.cjs.js) and in the polyfill modules (e.g., in polyfill/mod.rs). This design avoids duplicating the helper code in every output file.No modification is required on this front.
43-43:❓ Verification inconclusive
Destructuring exports without validation.
The code destructures
WorkerandreadFilewithout checking if they exist in the exports object. WhilereadFileis defined as a local variable in the module, it's not clear if it's properly exported. Consider adding validation.
🏁 Script executed:
#!/bin/bash # Check how these exports are used in other test files rg "Worker.*readFile" --include "*.{js,ts}" -A 5 -B 5Length of output: 497
Below is a revised verification step to help us further investigate the export usage. The original grep command failed due to an invalid flag; please run the following updated command to re-check where the
WorkerandreadFileexports might be referenced:If the output still yields no useful information, please perform a manual verification of the references to these exports. Once confirmed, consider whether adding validation for the destructured exports is necessary (for example, checking that
reexport_cjs()["Worker"]is defined before destructuring).Next Steps:
- Verify if other parts of the codebase reference these exports, confirming they are correctly structured.
- If you find that the exports may indeed be missing or misconfigured, add appropriate validation.
🏁 Script executed:
#!/bin/bash # Re-run search with valid file inclusion flags rg "Worker.*readFile" -g "*.js" -g "*.ts" -A 5 -B 5Length of output: 2306
Action Required: Validate and Safeguard Destructured Exports
The current implementation at
crates/compiler/tests/fixtures/bundle/export/doubleReexportAll/output.js(line 43) uses:var Worker = reexport_cjs()["Worker"], readFile = reexport_cjs()["readFile"];This direct destructuring assumes that both
"Worker"and"readFile"properties exist on the object returned byreexport_cjs(). However, as noted, there’s also a local declaration ofreadFile(e.g.,const readFile = 123;seen nearby), which makes it unclear whether the export fromreexport_cjs()is actually valid or if it might inadvertently shadow or conflict with the local variable.Recommendations:
- Validate Exports: Consider refactoring by first storing the result of
reexport_cjs()in a temporary variable and then verifying that the expected keys (especially"Worker") are present before assigning them.- Review Local Definitions: Double-check if the local
readFileis intentionally defined. If the intent is to use the exported value, ensure there is no conflict or ambiguity between the export and the local variable.- Error Handling: If a required export is missing, an explicit error or fallback behavior would improve the robustness of the module.
Please verify these areas manually in the context of the overall module behavior to ensure that the correction aligns with the intended design.
crates/compiler/tests/fixtures/bundle/library/hybrid/normal/output.js (4)
3-19: Be mindful of objects with multiple or zero keys in__commonJs.When
modis an object, the code picks the first property key. Ifmodhas multiple or zero keys, this could lead to unexpected behavior. Ensure this calling convention is valid for all users of__commonJs.
20-24: Well-implemented default import interop.The
_interop_require_defaultfunction is a standard approach to handle default imports from CommonJS modules. This looks correct and consistent.
25-29: Confirm Node environment dependencies.Using
createRequire(import.meta.url)is a valid approach but requires a Node environment that supports ES modules. Confirm whether your target environments meet these requirements.
70-71: Appropriate consolidated exports.Exporting both named and default members in a single statement is concise. This is a clear approach for bundling multiple members from one module.
crates/compiler/tests/fixtures/runtime/bundle/browser/normal/output.js (3)
19-19: No immediate issues.Defining a new module via
__commonJsis consistent. The"use strict";directive is fine, and logging is straightforward. No changes suggested here.
24-24: Explicit module invocation is correct.Invoking
runtime_ts_cjs()immediately triggers module execution. This is a valid approach for bundler-based flows. No adjustments needed.
2-18:❓ Verification inconclusive
Consider verifying object structure and improving naming for clarity.
The lazy initialization logic in
__commonJs()is straightforward for bundler code, but ifmodis an object with no keys, line 14 may throw an error. In practice, generated code likely ensures at least one key, but it's safer to confirm. Also, to avoid confusion with Node.js built-ins, consider renamingmoduleto something likecachedModule.Would you like to run a quick search confirming all generated code assigns at least one key to
modobjects?
🏁 Script executed:
#!/bin/bash # Script to verify if bundler-generated code always provides keys in the `mod` object # Searching for code constructs that define the `mod` object to ensure it has at least one key rg -A 3 "$'__commonJs\\(\\(module, exports\\)=>'"Length of output: 54
Verification of object structure and naming in
__commonJs()Based on the initial inspection, the concern remains that if
modis an object without any keys, usingmod[Object.keys(mod)[0]](...)would throw an error. Although generated code likely guarantees at least one key, our automated search didn’t produce conclusive evidence to fully confirm this. Additionally, the suggestion to rename the local variablemodule(e.g., tocachedModule) to reduce confusion with Node.js built-ins is still valid.Action Items:
- Verify Generated Code: Please manually confirm that every invocation of
__commonJs()(especially in files likecrates/compiler/tests/fixtures/runtime/bundle/browser/normal/output.js) supplies a non-emptymodobject.- Improve Naming: Consider renaming the local variable
moduleto something likecachedModuleto enhance clarity and avoid conflicts.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/star/output.cjs.js (2)
15-15: Verify behavior for modules with multiple or zero properties.
This line invokes the first key inmod:mod[Object.keys(mod)[0]](module, module.exports);If
modhas multiple or zero properties, it may behave unexpectedly. Consider handling edge cases by checking for additional or missing keys.
107-107: Module re-exports look correct.
The_export_starcall correctly re-exportsfoo_cjstomodule.exports. No concerns here.crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/output.cjs.js (5)
1-14: Bundle1.js implementation looks correct.This section correctly implements a CommonJS module that mocks a lodash library with a merge method. The module is wrapped using the
__commonJsfunction, which handles the module initialization and caching. The export pattern in line 10(module.exports = _)._ = _is a common pattern in libraries like lodash to make both the default export and named properties accessible.
18-34: Well-implemented CommonJS module factory function.The
__commonJsfunction correctly implements module caching (lines 21-23) and handles both function-style and object-style module definitions (lines 27-31). This matches the CommonJS specification and provides proper module initialization semantics.
35-39: Interop helper function looks good.The
_interop_require_defaultfunction correctly handles ES module interoperability by checking for the__esModuleflag and properly wrapping non-ES modules with a default property.
47-54: Entry point correctly uses the modules.The index.js section properly imports the necessary functions and modules, and demonstrates correct usage of both the default export and named export from the lodash mock library. The use of
_interop_require_defaultensures compatibility with both ESM and CommonJS modules.
42-44:❓ Verification inconclusive
Potential reference to undefined variable.
The code references
exportswhich appears to be undefined in this scope. While this might work in a Node.js environment whereexportsis implicitly available, it could cause issues in other contexts.
🏁 Script executed:
#!/bin/bash # Check if there's an exports declaration earlier in the file or in imported modules rg -A 2 -B 2 "var exports|let exports|const exports" crates/compiler/tests/fixtures/bundle/multiple_bundle/lodash_export/Length of output: 123
Attention: Verify the CommonJS module wrapper for
exportsAfter investigation, no explicit local declaration of
exports(viavar,let, orconst) was found in the file. This is expected for a bundled CommonJS module whereexportsis supplied by the module wrapper at runtime. However, please manually verify that the bundle indeed wraps the code in a manner that providesexports(e.g., as a function parameter or via the Node.js module system). If the file is executed in an environment that does not supplyexportsautomatically, then this could lead to runtime issues.
- Action Item: Confirm that the file's runtime environment (or wrapper) properly defines and supplies
exportsso that the use ofObject.defineProperty(exports, "__esModule", { value: true })is valid.crates/compiler/tests/fixtures/external/node/hosting/import/output.js (1)
19-19:__commonJsusage is concise and congruent with the intended lazy-loading design.
No evident issues arise from this initialization approach; it follows the same structure used in the rest of the codebase.crates/compiler/tests/fixtures/external/node/normala/esm/output.js (1)
19-19:__commonJsinstantiation appears correct.
No further changes are required here; this maintains consistency with the surrounding code.crates/compiler/tests/fixtures/external/node/hosting/require/output.js (2)
2-18: Verify assumptions about the shape ofmod.
Usingmod[Object.keys(mod)[0]]may behave unpredictably ifmodincludes additional keys or none at all. Confirm that this will not occur or implement error handling.
19-19: Module initialization aligns with existing patterns.
Instantiatingindex_js_cjsin this manner is consistent. No immediate performance or logical concerns found.crates/compiler/tests/fixtures/external/browser/normal/cjs/output.js (1)
19-19:index_js_cjsinitialization seems proper.
The callback approach in__commonJsis consistent with the broader code pattern. No issues to address.crates/compiler/tests/fixtures/external/node/normala/cjs/output.js (1)
19-19: LGTM!
Creation and invocation of theindex_js_cjsstub look consistent.crates/compiler/tests/fixtures/external/browser/normal/esm/output.js (1)
19-19: Looks good overall!
Module creation and invocation appear aligned with the browser flow.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/foo.ts (1)
2-3: Export style changed from direct string to variableThe export has been modified from directly exporting a string literal to using a variable. This change aligns with the broader refactoring of the module system in Farm v2.0, providing more flexibility when testing hybrid CJS/ESM interoperability.
crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/reexport.ts (2)
10-12: Exported class and constant structureThe export structure looks good, separating named exports and default exports clearly. This approach is consistent with modern JavaScript module practices.
4-8:⚠️ Potential issueSyntax error in class declaration
There's a syntax error in the class declaration - the semi-colon after the closing brace is unnecessary and not standard JavaScript/TypeScript syntax.
-class foo { - constructor() { - console.log(this.constructor === foo); - } -}; +class foo { + constructor() { + console.log(this.constructor === foo); + } +}Likely an incorrect or invalid review comment.
crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/index.ts (1)
1-4: Module structure refactoredThe module structure has been refactored to import from './reexport' instead of directly from './foo.ts', and re-exports both the default and all named exports. This change improves the organization by centralizing exports through the reexport module.
crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/default/output.js (2)
7-12: Output contains properly bundled class definitionThe bundled output correctly includes the
fooclass definition from the reexport module, maintaining the original functionality.
14-15: Export structure simplified in bundled outputThe bundled output has been significantly simplified compared to previous implementations, removing several interop helper functions while maintaining correct exports. This aligns with the PR objective of streamlining module export interop in Farm v2.0.
crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/namespace/output.js (8)
1-2: Headers look fine.
No functional logic is introduced here, so there's nothing problematic to flag.
3-19: Check for edge cases when calling non-function or multi-key modules.
While this bundler pattern is standard, consider handling scenarios whereObject.keys(mod)might yield multiple keys or no keys at all. Ensuring robust fallback paths and clear error reporting would improve maintainability.
20-27: Good caching strategy for wildcard interop.
This approach gracefully falls back tonullwhenWeakMapis unavailable, ensuring broader environment compatibility.
28-49: Typical Babel-style wildcard interop.
No obvious issues; this is a standard pattern for handling ES module and CommonJS interop in bundlers.
50-50: Exporting both helpers is consistent.
Exporting__commonJsand_interop_require_wildcardmatches the usage in subsequent code.
53-82: Implementation of_exportand module definitions is coherent.
The code properly defines read-only exports (foo,bar, and_default) and sets the CommonJS flag. It aligns with typical bundler transformations.
87-87: No actionable feedback on this comment line.
88-88: Re-export naming is straightforward.
Exportingfoo_nsasnsis clear and follows a conventional pattern.crates/compiler/tests/fixtures/bundle/library/reexport/reexport_hybrid_cjs/named/output.js (6)
1-19: Confirm fallback behavior.
Similar to the other file, ensure__commonJshandles cases with multiple or missing keys on themodobject, avoiding unexpected run-time errors.
20-20: Module export is consistent.
Exporting__commonJsalone here is sufficient and aligns with usage downstream.
23-24: Comment lines only.
No issues to address.
25-50: Reading and exporting named symbols is well-structured.
All transformations, including_exportand assignment tomodule.exports, look correct for CommonJS bundling.
53-53: Comment line only.
No functional impact, so nothing to adjust.
54-54: Straightforward named export.
This final export is appropriately concise.crates/compiler/tests/fixtures/partial_bundling/basic/output.js (6)
4-6: Lazy initialization logic looks sound.
This approach efficiently defers module creation until needed.
8-10: Initialization of themoduleobject is appropriate.
Creating and storingmodule.exportsaligns well with a CommonJS-style bundling pattern.
11-14: Validate the shape ofmodbefore calling.
Ifmodis an empty object or has multiple keys, themod[Object.keys(mod)[0]]call at line 14 could raise runtime errors or unexpected behavior.Would you like me to generate a script to confirm the usage patterns of
modacross the codebase to ensure they meet the expected structure?
16-16: Clean memoization pattern.
Returningmodule.exportson subsequent calls ensures only one initialization. Code is concise and readable.Also applies to: 18-18
22-24: Double-check plugin overwrite.
Setting the plugins array to[]will remove all previously registered plugins. Verify if this is the intended behavior.
25-25: IIFE usage is valid.
Wrapping in an immediately invoked function helps isolate scope and minimize global pollution.crates/compiler/tests/fixtures/runtime/bundle/cjs/export/exportStar/output.js (3)
2-18: Consider validatingmodinside__commonJs.While this lazy-initialization approach works, an empty or unexpected
modcould lead to out-of-bounds access onObject.keys(mod)[0].As a follow-up, consider adding a safeguard:
if (typeof mod === "function") { mod(module, module.exports); } else { + if (!mod || Object.keys(mod).length === 0) { + throw new Error("Invalid module object"); + } mod[Object.keys(mod)[0]](module, module.exports); }
19-34: Reusable namespace merging looks correct.
35-39: Straightforward export assignment.crates/compiler/tests/fixtures/runtime/bundle/cjs/export/exportNamed/output.js (1)
2-19: Reassess potential edge cases in__commonJs.The logic is consistent with other bundler code, but validating that
modalways has at least one key can help prevent runtime errors when callingmod[Object.keys(mod)[0]].crates/compiler/tests/fixtures/runtime/bundle/cjs/export/exportNamespace/output.js (4)
2-18: Add an optional check formodin__commonJs.As in the other files, consider validating that
modis non-empty before accessing its first key.
19-26: WeakMap usage and cache logic are fine.
27-48: Wildcard interop implementation is standard.
49-49: Module export initialization is straightforward.crates/compiler/tests/fixtures/runtime/bundle/cjs/export/entryExportStar/output.js (4)
2-18: New__commonJsfunction implementation looks good, but consider edge case handling.The new
__commonJsfunction provides a simpler approach to CommonJS module handling, with proper module caching. The implementation correctly handles both function and object module formats.However, there's an edge case in line 14 that assumes the module object has at least one key. Consider adding a defensive check:
- mod[Object.keys(mod)[0]](module, module.exports); + const keys = Object.keys(mod); + if (keys.length > 0) { + mod[keys[0]](module, module.exports); + }
19-26: LGTM: Properly implemented module cache with WeakMap.The
_getRequireWildcardCachefunction is well-designed with proper WeakMap usage to prevent memory leaks while caching module transformations. The conditional fallback for environments without WeakMap support is a good practice.
27-48: LGTM: The wildcard interop function handles all necessary edge cases.The
_interop_require_wildcardfunction properly handles:
- ESM modules with the
__esModuleflag- Null or primitive values
- Property descriptors with getters/setters
- Caching for performance
- Default export mapping
This is a solid implementation that maintains compatibility while improving the code structure.
49-50: Successfully replaced legacy module system with the new__commonJsimplementation.The changes show proper integration of the new module system, applying the wildcard import for Node.js modules and using the new
__commonJsfunction for the CommonJS module..github/workflows/rust-test.yaml (1)
6-6: Proper workflow configuration for v2 development branch.Adding the v2-dev branch to the pull request trigger ensures that Rust tests will run for PRs targeting this new development branch, maintaining consistent CI checks across both main and v2-dev branches.
crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/bundle2.ts (1)
1-6: Well-structured test fixture for hybrid module imports.The file properly exports constants and an empty default function, which will be useful for testing module bundling functionality.
crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/esm.ts (1)
1-4: Well-structured ES module test fixture.The file correctly implements ES module exports with named constants and a default function.
crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/cjs.ts (2)
1-1: Verify if the name value is correct.In other similar files, the name matches the file/module name (e.g.,
bundle2Name = 'bundle2'andesmName = 'esm'), but herecjsName = 'foo'. Is this intentional or should it be'cjs'for consistency?
1-4: Well-structured hybrid module test fixture.This file correctly demonstrates hybrid module exports mixing CommonJS and ES module patterns, which is useful for testing interoperability.
biome.json (4)
7-7: Update Default VCS Branch SettingThe
"defaultBranch"value has been updated from"main"to"v2-dev", which is in line with the other configuration changes (e.g., in.changeset/config.json).
51-56: Review Linter Rules in the "complexity" SectionThe settings for the following rules have been adjusted:
"noBannedTypes"is now set to"off"."noWith"is enforced as"error"."noUselessCatch"is set to"off".Please confirm that disabling or enforcing these specific linter rules is intentional, as it can impact code quality and consistency across the project.
78-78: Assess Disabling Unused Variables WarningThe
"noUnusedVariables"rule has been changed from a warning ("warn") to being completely turned off ("off"). Verify that this decision is deliberate to avoid noisy warnings during development and that it doesn’t unintentionally allow potential oversights in the code.
122-122: New Ignore Entry for Documentation FilesThe addition of
"**/docs/**"to the linter ignore list is meant to exclude documentation files from linting. Ensure that this aligns with the desired project workflow for keeping documentation style separate from code quality checks..npmrc (1)
5-7: New NPM Configuration SettingsThe following settings have been added:
fetch-retries=0tag-version-prefix=""git-tag-version=falseThese changes adjust how npm handles fetch retries and version tag creation. Please ensure these values are aligned with your release and versioning strategy.
.vscode/settings.json (2)
10-10: Update JavaScript Default FormatterThe default formatter for JavaScript has been updated from
"biomejs.biome"to"esbenp.prettier-vscode". This change should lead to a more standardized code formatting experience for JavaScript files.
17-20: Addition of YAML Formatter ConfigurationA new configuration block for YAML files has been added to set the default formatter to
"esbenp.prettier-vscode". This ensures consistent formatting for YAML files, similar to the JavaScript update..github/workflows/release.yaml (2)
38-38: Artifact Handling for Plugin Replace DirnameA new command has been added to move artifacts for the
plugin-replace-dirname(line 38). This enhances artifact management during the release process. Please verify that the destination path./rust-plugins/replace-dirname/npm/${abi}is correct and that the artifacts are handled as intended.
63-66: Conditional Versioning and Publishing Based on Branch NameThe configuration now conditionally sets:
version: usingpnpm run bump:nightlyfor thev2-devbranch orpnpm run bumpotherwise.publish: usingnpm run release:nightlyforv2-devornpm run releaseotherwise.branch: targetingv2-devwhen onv2-devand defaulting tomainotherwise.This breaking change enables nightly builds and standard releases based on the branch context. Please verify that the conditional operators and commands remain consistent with your overall release strategy.
CONTRIBUTING.zh-CN.md (1)
106-107: Addition of Snapshot Update Instruction in Rust TestingA new guideline has been added:
如果需要更新测试快照，你可以使用
INSTA_UPDATE=always cargo testThis instruction provides extra clarity on how to update test snapshots during Rust testing. Ensure that contributors are aware of this change and that it is documented consistently with the English CONTRIBUTING guidelines if applicable.
CONTRIBUTING.md (1)
109-114: Good documentation addition for snapshot testing!The added instructions clearly explain how to update snapshots for Rust tests, which is valuable information for contributors working with the test suite.
crates/compiler/tests/css.rs (2)
2-2: Module import change from standard library to farmfe_coreThe HashMap import has been changed from standard library to a custom implementation in farmfe_core.
17-17: HashMap initialization method updated to from_iterThe HashMap creation has been updated from
HashMap::from()toHashMap::from_iter()to match the API of the custom HashMap implementation.Also applies to: 43-43, 70-70
crates/compiler/src/build/finalize_module.rs (1)
12-12:✅ Verification successful
BREAKING: Parameter type changed to mutable reference
The
depsparameter is now a mutable reference, indicating the function may modify the dependencies during execution. This is a breaking change that will require updates to all callers of this function.
🏁 Script executed:
#!/bin/bash # Find all callers of finalize_module function to verify they're updated to pass mutable references rg "finalize_module\(" --type rust -A 3 -B 3 | grep depsLength of output: 571
Action Required: Confirm mutable reference usage for
depsacross all call sitesThe parameter
depsinfinalize_moduleis now defined as a mutable reference (&mut Vec<PluginAnalyzeDepsHookResultEntry>). This breaking change means that all callers must pass a mutable reference. Verification via the grep search confirms that the call sites—including those in:
crates/compiler/src/build/finalize_module.rs(during parameter initialization)crates/plugin_css/src/transform_css_to_script.rs(using&mut vec![])crates/plugin_script/tests/common/mod.rs(passing&mut deps)crates/plugin_script/tests/hmr_accepted.rs(using&mut vec![])—have been updated accordingly. Please ensure any additional call sites are similarly updated.
crates/compiler/src/utils.rs (1)
1-24: Well-structured utility function for error handlingThis new function extracts module IDs from different types of compilation errors, which will help with error tracking and reporting. The implementation properly handles different error types and extracts the relevant module information from each.
The function's design allows for graceful handling of various error types while ignoring those that don't contain module information.
.changeset/config.json (3)
8-8: Update to new development branch for v2.0.The
baseBranchhas been updated from "main" to "v2-dev", which aligns with the PR's focus on developing Farm v2.0. This ensures that changesets will be calculated relative to the new development branch.
10-10: Excluded packages from changeset tracking.The
ignorearray has been expanded to exclude example packages and documentation from the changeset process. This separation is appropriate as these components typically don't need versioning alongside the core packages.
11-13: Added snapshot versioning configuration.The new
snapshotconfiguration withuseCalculatedVersion: truewill enable more predictable version calculation for snapshot builds, which is helpful during the development of a major version update.crates/compiler/src/generate/mod.rs (2)
1-1: Standardized HashMap import from farmfe_core.Importing
HashMapfromfarmfe_coreinstead of the standard library ensures consistency with other parts of the codebase and potentially leverages any custom implementations or behaviors defined in the core module.
23-23: Consistent use of default() instead of new() for HashMap initialization.Changed from
HashMap::new()toHashMap::default(). While functionally equivalent for standard HashMap implementations, this change maintains consistency with the project's conventions for collection initialization.crates/compiler/src/update/diff_and_patch_module_graph/test_patch_module_graph.rs (3)
7-7: Standardized HashMap import from farmfe_core.Importing
HashMapfromfarmfe_coreinstead of the standard library ensures consistency across the codebase.
163-163: More specific edge validation in test.Changed from checking if an edge is dynamic to specifically verifying if it's a dynamic import. This makes the test more precise about the condition being validated.
421-497: Added comprehensive test for dynamic entry handling.This new test validates the behavior of adding and removing dynamic entries in the module graph. It ensures that dynamic entries are correctly tracked in the
dynamic_entriesmap when edges withDynamicEntrykind are added or removed during module graph patching.The test covers important edge cases including:
- Proper removal of dynamic entries when associated edges are removed
- Correct addition of new dynamic entries with specific naming
- Accurate updates to the module graph structure
This is valuable for ensuring the stability of dynamic entry handling during the v2.0 refactoring.
crates/compiler/src/update/find_hmr_boundaries.rs (4)
6-6: Standardized HashMap and HashSet imports from farmfe_core.Importing collection types from
farmfe_coreinstead of the standard library ensures consistency across the codebase.
13-13: Consistent use of default() instead of new() for collection initialization.Changed from
HashMap::new()andHashSet::new()to theirdefault()counterparts. While functionally equivalent for standard implementations, this maintains consistency with the project's conventions.Also applies to: 18-18, 24-24
28-28: Updated module ID resolution to use configuration mode.The code now explicitly uses
context.config.modewhen resolving module IDs, which ensures that the correct ID format is used based on the current compilation mode (development or production).Also applies to: 31-31
138-141: Consistent structure instantiation in test fixtures.The test fixtures have been updated to maintain a consistent approach to creating module metadata. This improves code clarity while maintaining the same test functionality.
Also applies to: 155-158, 161-164, 186-189, 203-207, 210-214, 236-240, 244-247, 261-265, 269-272, 276-279
crates/compiler/tests/fixtures/bundle/library/hybrid/esm_export_cjs/output.js (1)
25-34: Clean implementation of CommonJS module with proper exportsThe implementation correctly uses the new
__commonJshelper to define a module with propertiesnameandage, following standard CommonJS patterns. This is a good example of the new approach to handling module exports..github/workflows/ci.yaml (4)
6-6: Added v2-dev branch to PR workflow triggersThe addition of the v2-dev branch to the pull request workflow ensures that PRs targeting this branch will trigger the CI workflow. This aligns with the PR objective of Farm v2.0 development.
136-141: Added download step for plugin-replace-dirname in ts-test jobThis change mirrors the artifact download step added to the examples-test job, maintaining consistency across test jobs that require plugin artifacts.
204-204: Added plugin-replace-dirname to check-plugin-artifacts matrixThis addition ensures that artifacts for the new replace-dirname plugin are verified, maintaining consistency with the artifact checks for other plugins.
54-59:✅ Verification successful
Added download step for plugin-replace-dirname artifact
This change ensures that the replace-dirname plugin artifact is downloaded for the examples test job, consistent with how other plugin artifacts are handled.
🏁 Script executed:
#!/bin/bash # Check if the replace-dirname plugin directory structure exists in the repo fd --type d "replace-dirname" ./rust-pluginsLength of output: 76
Artifact download step verified and working as expected.
The download step for theplugin-replace-dirnameartifact in.github/workflows/ci.yaml(lines 54–59) correctly targets the npm package directory within./rust-plugins/replace-dirname. Our investigation using the provided shell script confirmed the presence of the./rust-plugins/replace-dirname/directory. This ensures consistency with how other plugin artifacts are handled and no adjustments are required.crates/compiler/Cargo.toml (2)
17-17:✅ Verification successful
Added farmfe_plugin_script_meta dependency
New dependency added for script metadata functionality. This appears to be part of the modularization of the compiler for v2.0.
🏁 Script executed:
#!/bin/bash # Check if this plugin exists and what it contains fd -t d "plugin_script_meta" --exec ls -la {}Length of output: 271
Dependency Confirmation for
farmfe_plugin_script_metaThe dependency added to
crates/compiler/Cargo.toml(line 17) correctly points to a valid local crate. The shell script verified that theplugin_script_metadirectory exists and contains the expected structure (aCargo.tomland asrcdirectory). This addition aligns with the modularization strategy for the compiler v2.0.
32-33:✅ Verification successful
Added new plugin dependencies for exports mangling and library support
Two significant new dependencies have been added:
farmfe_plugin_mangle_exports- Likely handles exports optimizationfarmfe_plugin_library- Provides library-specific functionalityThese additions align with the V2 development goals and replace the removed
farmfe_plugin_bundlementioned in the AI summary.
🏁 Script executed:
#!/bin/bash # Verify these plugins exist in the codebase and gather basic info for plugin in "plugin_mangle_exports" "plugin_library"; do echo "Checking $plugin..." fd -t d "$plugin" --exec ls -la {} doneLength of output: 858
Dependency Inclusions Verified and Approved
The new dependencies in
crates/compiler/Cargo.tomlare confirmed to be correctly set up:
- The code snippet below accurately reflects the new plugin dependencies:
farmfe_plugin_mangle_exports = { path = "../plugin_mangle_exports", version = "0.0.1" } farmfe_plugin_library = { path = "../plugin_library", version = "0.0.1" }- Both plugins were found in the repository with the expected structure (each contains a
Cargo.tomland relevant source files).- These changes align with the V2 development goals and appropriately replace the previously removed
farmfe_plugin_bundledependency.No further modifications are necessary.
crates/compiler/src/update/diff_and_patch_module_graph/test_diff_module_deps.rs (1)
7-7: Improved imports by using HashMap and HashSet directly from farmfe_coreImporting
HashMapandHashSetdirectly fromfarmfe_coreinstead of fromstd::collectionssimplifies the import structure and potentially provides custom implementations optimized for the project.crates/compiler/src/generate/resource_cache.rs (1)
78-86: Adopting ownership forresourceparameter
Changing the function signature to acceptresourceby value (instead of by reference) appears consistent with avoiding unnecessary clones and streamlining resource management. This should be beneficial as long as the caller no longer requiresresourceafterward, ensuring no unintended aliasing or reuse issues.crates/compiler/src/update/diff_and_patch_module_graph.rs (13)
3-3: Good use ofVecDeque
AdoptingVecDequehere is an appropriate choice for queue-like operations (e.g., BFS) due to its efficient push/pop.
10-10: Additional import forResolveKind
IntroducingResolveKindsuggests further usage or checks on module resolution logic. No issues noted.
12-12: AddedHashMapandHashSetimports
Including these standard collections is consistent with the subsequent usage.
98-99: InitializedHashSetusing default
UsingHashSet::default()in place ofHashSet::new()is a stylistic change and functionally equivalent.
137-138: Use ofHashMap::default()
Same observation here: switching to.default()maintains clarity and consistency with the rest of the file.
141-153: Dynamic entry removal logic
Removing the dynamic entry if no other dependents contain a dynamic reference is a logical and clean approach to keep the module graph accurate. Ensure no concurrency hazards arise from parallel modifications todynamic_entries, but as written, it looks correct.
219-219: Returning default sets
Returning(diff_result, HashSet::default(), HashSet::default())maintains consistent usage of default initializers.
272-274: New default initializations
Creating these maps and sets via default is stylistically consistent.
315-315: Using.or_insert_with(HashSet::default)
This is a more efficient approach to insert only when needed, preventing unneeded allocations.Also applies to: 328-328
345-345: Returning default on empty diff
Provides a clear, consistent pattern when nothing is changed.
382-382: UsingHashSet::default()foradded_visited
Maintains style congruence and clarity.
403-403:.or_insert_with(HashSet::default)
Similar to other segments, this is a concise, lazy initialization for the set.
431-431:removed_visitedinitialization
Consistently uses default initialization.crates/compiler/tests/common/mod.rs (4)
17-17: ImportingHashMap
ActivatingHashMapimport aligns with the updated usage in the file.
128-128: Switching toHashMap::default()
No functional difference versusnew(), but consistent with the broader style changes.
342-348: Detailed assertion error messaging
Including both expected and actual results in the assertion improves debuggability when test outputs differ.
468-468: AdoptingHashMap::from_iter
This cleanly constructs the input map from a single key-value pair.crates/compiler/tests/fixtures/external/browser/object/esm/output.js (1)
2-20: Simplified__commonJsfunction
The new approach of using an IIFE and a single__commonJswrapper clarifies module initialization and export handling. This refactor removes extraneous interop functions and appears to streamline the code while preserving functionality.crates/compiler/tests/fixtures/bundle/library/hybrid/hybrid_entry_should_reexport/output.js (2)
3-19: Ensure safe fallback for empty or unexpectedmod.
This bundler helper might throw an error ifmodlacks any keys. Consider validating or handling an edge case wheremodis empty, to avert potential runtime errors.
28-47: Re-check usage offs.read.
fs.readtypically requires a file descriptor and is less common compared tofs.readFileorfs.readFileSync. Ensure this is the intended method or switch to a more standard approach.crates/compiler/tests/fixtures/bundle/library/hybrid/importZip/output.js (3)
1-6: All good—approved.
The exported constants and function inbundle1.jslook correct for a test fixture.
9-34: Bundler helpers appear correct.
__commonJsand_interop_require_defaultare standard bridging utilities. No issues found.
36-100: Mixed module usage is logically consistent.
The test code demonstrates both CJS and ESM interop. Everything seems fine for verifying bundler behavior.crates/compiler/src/update/patch_module_group_graph/test_dynamic_entries.rs (3)
1-12: Imports are well-organized.
No correctness issues detected.
19-159: Verify expectations for removed modules and final entries.
You remove"B"inupdate_module_graph, yet the final assertion expects"B"to remain inmodule_graph.entries. Confirm that preserving"B"aligns with your intended partial-updates logic.
161-273: Overall test flow looks correct.
The second test similarly verifies dynamic entries and partial module removal. The logic is consistent with the patching approach.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (6)
10-11: Imports look appropriate.
These added imports (ModuleGroupId,ModuleGroupType,ModuleType, andHashSet) appear consistent with the usage below and help ensure type safety in the updated assertions.Also applies to: 14-14
37-43: Method call for adding module "H" looks correct.
UsingModuleType::Jsclearly distinguishes the new module's purpose. Ensure that future code references or tests also reflect this new module type.
73-77: Clear creation of module group IDs.
Defining all relevant module group IDs upfront helps readability. However, ensure that these IDs remain in sync with any changes to the underlying module naming conventions in future refactors.
80-80: Unified use ofHashSet::from_iter.
Replacing literal strings or separate group IDs with aHashSetis a clean approach for set-based comparisons. This ensures consistent ordering is not necessary.Also applies to: 94-94
141-141: Check newly enforced resource pot naming.
The test expects"test_js"as the enforced resource pot. Confirm that this label aligns with the end-to-end system naming and that no mismatch occurs elsewhere.
201-203: Accurate assertion for newly tracked group in dynamic import.
Creating and assertinggroup_id_iensures the dynamic import is recognized. This is a good addition for verifying partial bundling logic.crates/compiler/src/generate/finalize_resources.rs (2)
4-15: Refined imports.
Bringing inMode,ModuleGroupId,ModuleGroupType,PluginFinalizeResourcesHookParam,PluginHandleEntryResourceHookParam, andHashMapis consistent with the new resource-processing flow. Ensure that all references to these newly imported entities are fully tested for correctness.
22-23: Delegating entry resource handling tohandle_entry_resource.
Extracting this logic into a dedicated function improves readability and maintainability offinalize_resources. It also cleanly separates entry-handling logic from post-processing steps.Also applies to: 24-28
crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (2)
19-19: CJS wrapper invoked immediately.
Callingindex_js_cjs();on creation ensures the runtime code runs at the correct initialization point. This is consistent with typical module bootstrap in most bundlers.
26-26: Check usage ofmodule._m(exports).
The_mfunction is not shown in the snippet but is presumably generated by the bundling system. Confirm that_mhandles exports consistently across different module definitions and doesn't lead to unexpected side effects.crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
2-19: Ensure robust handling of object-based modules.
While callingmod[Object.keys(mod)[0]](module, module.exports)works for single-key objects, consider gracefully handling empty or multiple-key scenarios to avoid unintended behavior or errors if the module structure changes.crates/compiler/src/build/mod.rs (2)
26-26: No issues found in the import and module references.
These updated references to finalize the module graph and the new usage ofHashMapappear consistent and correct.Also applies to: 32-34, 41-41, 47-47, 62-62, 64-64
113-119: Module graph stats update looks fine.
The logic for setting module graph statistics and entries is straightforward and does not show any obvious pitfalls.crates/compiler/src/generate/render_resource_pots.rs (1)
91-95: Guard against missing entries in both maps.
Calling.unwrap()on the result of.or_else(|| dynamic_entries.get(name))may panic if neither map contains the key. Consider adding a fallback or an error message for missing entries.crates/compiler/src/update/mod.rs (22)
1-1: No issues with the new import.
This addition is standard practice for leveragingArc.
7-10: Added module references look good.
These imports seamlessly integrate relevant entities from themodulenamespace.
17-17: Map structures are appropriately introduced.
UsingHashMapandHashSetis typical for storing module relationships.
20-20: Resource retrieval utility import is fine.
This function appears necessary for dynamic resource operations.
21-23: Final module graph references are properly introduced.
Importingfinalize_updated_module_graphandfreeze_module_of_affected_module_graphaligns with the overall update flow.
47-47: New module inclusion is clear.
Declaringmod finalize_module_graph;helps isolate finalization logic.
84-89: Switching to.statsusage is consistent.
Replacing the former logic withstatscalls clarifies metric collection.
98-110: Adding plugin hook stats for module group graph.
This refactor records additional data about the HMR update flow. The approach looks correct.
124-141: HMR diffAndPatchContext stats capturing is well-structured.
Storing output and durations in plugin hook stats fosters traceability.
155-156: HMR stats initialization is clear.
Invokingadd_hmr_compilation_statsandset_start_timeensures the tracking starts at the correct phase.
174-182: Lock usage is controlled but verify concurrency in downstream calls.
Lockinglast_fail_module_idsand passing it tohandle_update_modulesis reasonable; dropping the lock promptly helps avoid contention. Confirm that threads insidehandle_update_modulesdon’t require a longer lock span.
233-248: End times are set properly in the error and success paths.
Ensure the separation ofbuild_end_timevs.end_timeis intentional. The usage here helps finalize stats for both partial and complete flows.
251-251: Double-check repeated build-end time calls.
You are settingbuild_end_timeabove in the error block and again here. Confirm this duplication is intentional or factor out the logic if needed.
268-269: Freezing affected modules is correctly placed.
It prevents further modifications to impacted modules after the HMR update cycle.
275-278: Finalizing updated module graph.
This call ensures an updated structure is produced before generating or re-generating resources.
323-332: Regenerate resources is well orchestrated.
Passingaffected_module_groups,previous_module_groups, and the callback provides a clean mechanism for updating dynamic content.
344-344: Return of updated result is clear.
Finalizing withOk(update_result)is straightforward and consistent.
571-571: Return type changed to module group IDs.
This shift toHashSet<ModuleGroupId>might be a breaking change if the signature was publicly exposed. Document it if needed.
658-658: Creating local dynamic_resources map.
This is normal practice; no concerns here.
663-663: Constructing aModuleGroupIdfor each entry is appropriate.
The approach ensures distinct grouping by entry.
677-677: Stats end time in synchronous path.
Finalizing the update withset_end_timeis coherent once all steps complete.
697-697: Stats end time for async path.
Likewise, calling this in the spawned thread is correct to mark completion..github/workflows/rust-build.yaml (8)
24-24: Added build command forreplace-dirnameon x86_64-unknown-linux-gnu.
This ensures the new plugin is included in the Linux GNU build.
40-40: Added build command forreplace-dirnameon x86_64-unknown-linux-musl.
Ensures a musl build is also created for that plugin.
65-65: Added build command forreplace-dirnameon i686-pc-windows-msvc.
Gives Windows 32-bit support for the new plugin.
77-77: Added build command forreplace-dirnameon aarch64-pc-windows-msvc.
Ensures coverage on Windows ARM64 builds.
125-125: PNPM cache setting is now explicit.
Switching to"pnpm"is clear and consistent with other steps.
159-159: Additional default build step forreplace-dirname.
This ensures the plugin is built outside of Docker if needed.
186-190: Artifact upload step forplugin-replace-dirname.
Publishing the newly built plugin artifact is correct to ensure availability.
235-235: Using--no-frozen-lockfile --ignore-scriptsfor FreeBSD.
This might be necessary for environment compatibility. Confirm that ignoring scripts doesn’t omit important setup steps.crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
2-20: Simplified CommonJS wrapper logic.
The newly introduced__commonJsfunction uses a closure to cache the module and returnmodule.exports. This tidy approach eliminates prior clutter with interop functions. No issues identified.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (4)
48-65: Ensure the dynamic entry logic is correct
This addition of module "I" as a dynamic entry from "E" and updating the edge from "I" to "F" seems logically consistent. However, please verify that any existing entry or dynamic import references to "I" in the rest of the codebase are updated accordingly to avoid potential graph inconsistencies.
88-102: New module group ID for dynamic entry
The creation ofgroup_id_ias aModuleGroupId::new("I", ModuleGroupType::DynamicEntry)and including it in the affected groups set aligns well with the newly introduced dynamic entry logic.
242-246: Consistent sorting before assertions
Sorting resource pots by ID prior to asserting withassert_resource_pots!ensures stable test outputs. This practice is helpful in preventing non-deterministic test failures.
279-283: Second test logic for single changed module
Registering module "I" as aDynamicImportgroup and confirming that only the new group is affected is a valid scenario. The approach cleanly isolates changes to a single module.Also applies to: 301-303
crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
2-19: Consolidated__commonJslogic
This IIFE and__commonJsfunction simplify the module-loading flow significantly, removing the need for multiple helper interop methods. The pattern of checking if the module is already loaded before returningmodule.exportsis sound and aligns with typical CommonJS-style bundling.crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
2-19: Streamlined module handling with__commonJs
Similar to the other file, this unified approach for module loading removes unnecessary interop utilities. The implementation is clear and should be easy to maintain.crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/output.js (9)
1-19: Consider potential edge cases in__commonJsfunction.
The function returns a closure that caches the module exports in a local variable. While this works as an optimization, line 15 accessesmod[Object.keys(mod)[0]], which assumes a single key or that the first key is the correct entry function. In scenarios where the object might have multiple keys or a non-deterministic key order, this could fail.
20-27: WeakMap usage in_getRequireWildcardCachelooks fine.
The caching strategy for nodeInterop vs. Babel interop is implemented with separate WeakMaps. This approach is a standard Babel-inspired pattern, and it appears correctly structured for the intended usage.
28-49: Validate wildcard import object creation in_interop_require_wildcard.
This function correctly returns early if the imported object is already an ES module or is non-object. The dynamic creation ofnewObjwith__proto__set to null helps avoid prototype pollution. The property descriptor logic looks correct for preserving getters and setters.
50-54:_interop_require_defaultusage aligns with Babel’s default import pattern.
The function checksobj.__esModuleand, if absent, wraps it in a default property. This matches typical Babel interop for default exports.
55-55: Export statement is well-structured.
The named exports facilitate direct imports in the consumer code. No issues identified.
58-67: Check naming consistency forlodash.tsreferences.
The snippet redefines a local varlodash_cjs, referencing"lodash.ts"in the object literal. Ensure that further references in the bundling process consistently use the same file ID string to avoid injection or naming conflicts.
69-69: Comment clarifies module identity.
The inline comment improves readability of the test lines below. No action required.
74-74: Ensuring consistent inline module annotations.
The comment format is consistent with the earlier pattern and helps track module IDs in tests or generated code.
77-77: The final log statement forindex.tsis consistent.
This line confirms thatlodash$1is recognized at runtime. No issues identified.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_dynamic_entry_resource_pots.rs (3)
1-16: Imports and context setup look correct.
The file correctly imports essential items (e.g.,Arc,CompilationContext, relevant plugin) and uses them in the test. No issues found at this stage.
17-24: Reference tohandle_dynamic_entry_resource_potsis consistent.
These lines confirm the usage of the newly introduced logic for dynamic entry resource pots. The references togenerate_resource_pot_mapandpatch_module_group_graphare well-structured.
26-146: Test logic is comprehensive but lengthy.
This test sets up two module graphs, applies changes, diffs them, and checks the resulting groups and modules. It demonstrates end-to-end coverage of dynamic entry resource pot handling. While it’s thorough, consider splitting it into smaller tests for more granular coverage.Other observations:
• The repeated usage of.unwrap()is acceptable in a test context but could result in partial logs if an error arises mid-test.
• The final assertion thatdynamic_entry_resource_potscontains exactly["BH_js"]indicates the new dynamic entry was correctly introduced.Overall, the test effectively verifies the changes.
crates/compiler/src/build/module_cache.rs (3)
5-5: Imports streamlined and expanded appropriately.
The addition ofresolve_module_markfromswc_try_withis logical, and consolidatingHashMapandHashSetis neat. The changed lines maintain clarity with no reported conflicts.Also applies to: 10-11, 13-13
245-255: Recreatingsource_mapmight have performance implications.
Here, you create a new SWC source map for each cached module upon retrieval. This approach is correct if ephemeral data is required for transformations, but be mindful of potential overhead in large-scale projects.
289-289: Resource pots reset to default.
Callingcached_module.module.resource_pots = Default::default()ensures they’re re-derived once the module is re-processed. This seems correct for forcing a fresh resolution.crates/compiler/tests/fixtures/css/modules/global/output.js (3)
2-18: EnsureObject.keys(mod)[0]is always valid.
In__commonJs(mod), the logic for handling non-function modules relies onObject.keys(mod)[0]. Confirm thatmodalways has at least one key to avoid potential runtime errors when referencing index 0.
19-19: Looks good!
This line correctly captures the bundled module and exports.
32-35: Verify the.baseproperty usage.
Currently,exports.defaultis an empty object, so referencing.basemay returnundefined. Confirm if this is intentional or if values are missing.crates/compiler/src/generate/partial_bundling.rs (6)
62-70: Good approach to handle dynamic entries separately.
Deferring the extension ofresources_potswithdynamic_entry_resource_potsuntil after filling necessary fields helps keep the logic streamlined and reduces duplication.
121-141: Confirm skipping dynamic entries here is correct.
The code intentionally ignoresModuleGroupType::DynamicEntrywhen filling necessary fields, relying on subsequent logic to fill them. Double-check that no critical data goes uninitialized.
185-199: Enforce resource pots logic looks consistent.
Excluding dynamic entries from enforced resources aligns with the newly introduced dynamic pot handling. No issues noted.
256-286: Consider notifying on missing dynamic entries.
Ifdynamic_entrieslacks an entry formodule_group.entry_module_id, the function silently returnsNone. If this scenario is unexpected, consider logging a warning or error for easier debugging.
300-310: Tests updated with partial bundling changes.
Using.into_keys()is concise and clear. The new approach correctly collects modules for generating module group graphs.
349-350: No concerns with the external module handling test.
The extension to skip external modules is verified, and the final assertion ensures resource pots remain consistent.Also applies to: 377-377
crates/compiler/tests/fixtures/css/modules/local/output.js (4)
2-18: EnsureObject.keys(mod)[0]is valid.
Similar to the global output,__commonJs(mod)depends on at least one key inmod. Verify that consumer code never invokes this with an empty object.
19-19: Module wrapper initialization looks correct.
No issues noted—the bundling logic is consistent with similar files.
26-33: Props for defining explicit export mappings.
Assigning these named class strings toexports.defaultmakes the.base,.hello, etc. properties readily accessible.
37-40: Correct usage of.baseproperty.
Unlike the global output,.baseis actually defined here, so logging it will not cause unexpectedundefinedbehavior.crates/compiler/src/lib.rs (14)
15-16: Imports forModuleIdandMutexlook correct.
They will be used to track failed modules in a thread-safe manner.
22-22: ExportingRUNTIME_INPUT_SCOPEfor broader usage.
This public export helps unify references to runtime scope.
28-28: Newutilsmodule introduced.
Encapsulating helper functions in a dedicated module can improve overall maintainability.
32-32: Storinglast_fail_module_idsin a Mutex is sensible.
Ensure any concurrent writes or access patterns properly handle synchronization to prevent race conditions.
38-42: Dynamic selection of the render plugin.
This conditionally creates eitherFarmPluginLibraryorFarmPluginRuntime, aligning with different output targets. Seems logically consistent.
45-46: Ensuring that script meta features plugin precedes the render plugin.
The comment clarifies the intended execution order. No issues observed.
61-63: AddingFarmPluginScriptMetain plugin list.
It will be executed before subsequent plugins. Looks aligned with the new architecture.
80-85: Enforcing script meta exports plugin ordering.
Running it after tree shake and before mangle exports is a typical pattern to ensure correct symbol usage.
91-99: Conditional insertion of the mangle exports plugin.
Guarding withif options.mangle_exportsis appropriate to avoid unnecessary transformations when not needed.
126-126: Initializinglast_fail_module_idsto an empty list.
Makes sense for capturing modules that fail post-initialization.
153-153: Set compilation start time at the beginning ofcompile().
This practice supports more accurate performance metrics instats.
168-168: Marking build end time.
Helps track the build phase duration distinctly from the rest of compilation.
178-178: Invoking.finish(...)for plugin lifecycle completion.
Ensures final plugin hooks run alongside updatedstats.
199-199: Setting overall end time after all compilation stages.
Cleanly finalizesstatsmeasurement.crates/compiler/src/update/patch_module_group_graph/tests.rs (18)
4-4: Added imports forModuleGroupIdandModuleGroupType.
Brings clarity and type safety for referencing module groups in the tests.
8-9: Expanded imports forHashMapandHashSet.
Using standard collections is straightforward for grouping. No issues identified.
10-13: New references to partial bundling helper functions.
These test utilities align with the integrated approach to building module group graphs.
75-82: Replaced string literals with typedModuleGroupIdentries.
This improves type safety and readability of the test with clear group identifiers.
90-99: Verifyingmodule_groupsfor each module.
The usage ofHashSet::from_iter([group_id_a.clone()])is consistent. No issues found.
153-159: Adding dynamic entry group for “F”.
Checking the new group ID logic ensures that changes in dynamic entries propagate correctly.
161-161: Retrieving a module group by ID.
Explicitly verifying the group’s members withmodule_group_b.modules()helps maintain test clarity.
175-175: Testing combined group membership formodule_a.
Effective check to confirm that modules inherit group IDs from multiple entry points or dynamics.
178-181: Confirmingmodule_b.module_groups.
Ensures the module belongs only to the expected entry group.
200-200: Removing module "C" from the update graph.
The patch tests still keep other references valid. Good coverage for partial removal logic.
217-217: Using.into_keys()for extracting entry keys.
Neat approach to gather all existing entries in the updated module graph.
262-262: Asserting the updated module still belongs to the correct group.
Keeping consistent test logic ensures accurate patching checks.
548-548: Introducing new test to remove normal edges.
Extends coverage of removing edges without dynamic imports, covering typical usage patterns.
550-550: New scenario:test_patch_module_group_graph_remove_normal_edge.
Covers removing normal edges and verifying module group integrity. Good addition to test coverage.
646-646: Test for removing a dynamic import edge.
Checks how dynamic group membership is adjusted when an edge is removed.
680-680: Ensuring dynamic group “G” and “F” updates intest_patch_module_group_graph_remove_dynamic_import_edge.
Verifies that leftover modules remain in correct groups after removal.
739-739: Added test for updating dynamic imports.
This confirms that switching an import from static to dynamic is handled properly by the patch logic.
790-790: Checking group membership changes withgroup_id_a,group_id_b, etc.
Ensures the patch logic respects updated import modes.crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
2-19: New__commonJswrapper simplifies module loading.
Eliminating multiple interop helpers decreases complexity and improves maintainability.crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (5)
2-6: Consider handling multi-key objects in__commonJs.Here, the code calls
mod[Object.keys(mod)[0]](module, module.exports). Ifmodis an object with multiple keys, only the first key is used. If that’s intentional, it might be worth documenting. Otherwise, you may need to iterate over all keys or specify which key to load.
8-16: No immediate issues noted for the module initialization logic.The closure-based approach for caching the
moduleobject is reasonable for single-threaded execution environments. No concurrency pitfalls are evident at first glance.
19-19: Confirm strict mode impact on bundler logic.Line 20 sets
"use strict";. Verify if there are any references or calls in the code that need non-strict behavior. Typically it’s fine, but double-check external dependencies.
26-26: Check the usage ofmodule._m.
module._m(exports)is non-standard naming. If it’s intentionally part of the bundler’s internal API, consider adding comments documenting its behavior.
29-29: Ensure resource map correctness.Double-check that
window['__farm_default_namespace__'].__farm_module_system__references are valid and consistent across modules. Typos or case differences in property names can break the module registry.crates/compiler/src/update/regenerate_resources/mod.rs (5)
3-9: Imports look consistent.The newly added imports for sourcemap handling, enhanced magic string features, and plugin hooks align with the refactor. Be sure all unused imports are removed if no longer needed.
14-17: Additional type usage fromfarmfe_core.Resource pot references and module data structures here look consistent. No obvious naming mismatches. Continue ensuring references remain in sync with other modules in the codebase.
21-23: Cross-file references in generate.The references to
render_resource_pots_and_generate_resourcesandrender_resource_pot_generate_resourcesmust remain consistent with any renames or refactors in thegeneratemodule.
185-198: Verify resource re-registration.The code removes resources from
resource_mapsand clears them fromresource_potfor updated modules. Ensure downstream logic re-registers them to avoid missing resources after updates.
234-235: Ensure no references remain to cleared resource pots.By setting
module.resource_pots = Default::default(), references in other data structures might become stale. Confirm there are no leftover pointers that expect a resource pot ID.Also applies to: 242-242
crates/compiler/tests/fixtures/bundle/library/hybrid/hybrid_entry_should_reexport/output.cjs.js (4)
1-19: Implementation of__commonJs,_export_star, and_interop_require_default.These helper functions handle CommonJS interop. The patterns here look consistent with typical bundler code. Just ensure
_export_starproperly enumerates only the intended keys (skips special internal properties).
38-43: Consolidate exports acrossmodule.exportsandexports.We have
module.exports.__commonJs = __commonJs;and alsoObject.defineProperty(exports, "__esModule", { ... }). Ensure the final consumer environment is consistent in how it references these exports.
52-68: Module-level side effects.Here we see a direct call to
console.log(fs.read, os.cpus)upon requiring. This can cause side effects. If that’s expected, ensure the bundling logic manages it properly; if not, consider lazy evaluation.
70-75: Consistency of re-exports.
_export_star(index_cjs(), module.exports)merges properties intomodule.exports. Confirm it matches the intended re-export structure (e.g.,module.exports.namefrom line 71).crates/compiler/src/update/handle_update_modules.rs (8)
6-6: Check for unused imports.
ThePluginUpdateModulesHookParam,UpdateResult, andUpdateTypeimports are introduced here. Please verify if all of them are used within this file or in related functionality, and remove any imports that remain unused.
15-15: Validate parameter usage.
The new parameterlast_fail_module_idshelps track modules that failed previously. Ensure all call sites correctly supply the intended module IDs, particularly if the code extends to incremental build or partial rebuild scenarios.
31-31: Good addition for re-processing failed modules.
Resolving and re-including last failed modules in the update flow is valuable for consistent error recovery. Great job!
39-39: Stats usage alignment.
Replacing or renaming the oldrecord_managerwith.statsis consistent. Confirm that downstream code referencing the old field is updated to avoid runtime errors.
100-100: Stylistic change todefault().
UsingHashMap::<String, Vec<String>>::default()is more idiomatic in Rust thannew(). This is a good practice for consistency and clarity.
171-171: Consistent stats tracking.
Continuing the stats usage is consistent with the earlier change. Please ensure all plugin hooks consistently log relevant inputs and outputs for better observability.
188-188: Validate final outcome.
Ensure that any subsequent logic or plugin hooks do not rely on a previousOk(result)with a different shape. Verify test coverage for partial failures or empty paths.
239-239: New helper function introduced.
Theresolve_last_failed_module_pathsfunction is a sensible addition.crates/compiler/src/update/finalize_module_graph.rs (2)
1-13: Review imports and references.
All newly introduced imports (farm_profile_function,PluginFreezeModuleHookParam, etc.) appear necessary. Confirm that any newly introduced dependencies are added to cargo manifests and that references to_supermodules remain valid.
14-32: Finalize updated module graph logic.
Thefinalize_updated_module_graphfunction properly triggersmodule_graph_updatedplugin hooks with added, removed, and updated modules. This design fosters extensibility for third-party plugins. Ensure that the returned error, if any, is correctly handled upstream to avoid partially updated states.crates/compiler/tests/fixtures/bundle/library/hybrid/importCjsNamespace/output.cjs.js (5)
1-19: Encapsulated CommonJS loading logic.
The__commonJsfunction is well-structured and checks if the module is already initialized, ensuring a single-load pattern. Make sure any potential caching side effects are understood (e.g., repeated calls returning the same export object).
20-27: Wildcard cache function.
Using aWeakMapto cache interop objects is a common pattern. As an extra safeguard, consider verifying edge cases when environment does not supportWeakMap, although your fallback tonullis likely sufficient.
50-54: Preserve default export semantics.
_interop_require_defaultensures the object is wrapped with adefaultkey when not already an ES module. This is a classic Babel/TypeScript pattern. Looks good.
58-63: Clarify ESM usage.
Settingexports.__esModule = truecan help with tooling that differentiates CommonJS from ES modules. Confirm if your build chain or plugin expects additional fields (e.g.,Symbol.toStringTag).
63-85: Check host environment compatibility.
- Requiring
farm_runtime.jsfrom within the same directory is standard.- Validate that
_interop_require_*usage remains consistent across all imported modules (especially for.tsor.cjs.jsexpansions).- The logs in
index.ts,a.ts, andb.tsproperly demonstrate how these references flow.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (4)
90-100: Double-check skipping of dynamic entry module groups.The logic deliberately ignores
ModuleGroupType::DynamicEntryinget_affected_modules(). Confirm that separate handling of these dynamic entries is indeed intended, as any missed modules might lead to partial bundling issues.
240-303: Verify consistency in handling dynamic entry resource pots.The
handle_dynamic_entry_resource_pots()function purges old resource pots (remove_resource_pot()) and then adds new ones. Ensure that partially updated data structures are not left in an inconsistent state if an error occurs during the process.Would you like a shell script to grep for all calls to
handle_dynamic_entry_resource_potsand confirm their usage? Let me know if so.
374-400: Review concurrency aroundremove_resource_pot().The function modifies multiple data structures (resource pot map, resource map, module graph) in sequence. If external parallel operations occur, a lock or synchronization is needed to avoid race conditions.
404-407: Good addition of test modules.No issues spotted. Ensuring coverage for dynamic entry resource pot logic is useful.
crates/compiler/src/build/finalize_module_graph.rs (5)
17-34: Finalize hook usage looks correct.The
finalize_module_graph()function properly callsmodule_graph_build_endand updates the execution order. This appears consistent and safe.
36-48: Consider verifying cache integrity.
cache_module_graph()merely collects and stores module IDs. If the set of module IDs changes, ensure there is no mismatch in subsequent caching logic.
50-77: Parallel freezing of modules.
freeze_module_of_module_graph()locks the module graph, but concurrency complexities may arise if other threads attempt to mutate state. Validate that no further write operations occur concurrently.
99-114: Graceful handling of unchanged modules incall_freeze_module_hook().If the module did not change, the function returns without calling
freeze_module. This is efficient. Ensure no side effects are needed for unchanged modules.
118-132: Parallel iteration incall_freeze_module_with_params().The hook params are processed in parallel. Confirm that the freeze hook is thread-safe and does not depend on shared mutable state unguarded by locks.
crates/compiler/src/update/patch_module_group_graph.rs (5)
21-21: Check downstream usage of the new return type.
patch_module_group_graph()now returnsHashSet<ModuleGroupId>instead ofHashSet<ModuleId>. Ensure all callers handle the updated type.
32-57: Potential infinite traversal risk.The loops for adding and removing dynamic import edges can recursively call each other. While it seems well-structured, confirm that repeated additions or deletions cannot cause indefinite loops.
101-157:add_dynamic_module_group()logic.Creating new groups for dynamic imports/entries is correct, but be cautious about concurrency if the module group graph can be mutated in parallel. A blocking write lock or an RwLock is presumably needed.
159-182:remove_dynamic_module_group()approach.After removing modules from the group, the code also removes the group if the module group is empty. Verify no references remain in parent or child edges.
461-571:patch_existing_removed_non_dynamic_children()is symmetrical to the added logic.No major findings; structure is consistent with additions. Just ensure both paths remain in sync as features evolve.
| export { a, b } | ||
|
|
||
| export * from './a.js'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing re-export from b.js
You're re-exporting from a.js but missing a similar re-export for b.js. According to the documentation comments, both modules should be re-exported.
export { a, b }
export * from './a.js';
+export * from './b.js';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export { a, b } | |
| export * from './a.js'; | |
| export { a, b } | |
| export * from './a.js'; | |
| export * from './b.js'; |
| function loadFoo(foo = defaultFoo) { | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix the empty return statement.
The return statement without a value and missing semicolon could lead to unexpected behavior with JavaScript's automatic semicolon insertion.
function loadFoo(foo = defaultFoo) {
- return
+ return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function loadFoo(foo = defaultFoo) { | |
| return | |
| } | |
| function loadFoo(foo = defaultFoo) { | |
| return; | |
| } |
| .meta | ||
| .set_globals(&cached_module.module.id, Globals::new()); | ||
|
|
||
| // using swc resolver | ||
| match &mut cached_module.module.meta { | ||
| box farmfe_core::module::ModuleMetaData::Script(script) => { | ||
| // reset the mark to prevent the mark from being reused, it will be re-resolved later | ||
| script.top_level_mark = 0; | ||
| script.unresolved_mark = 0; | ||
| // reset the mark to prevent the mark from being reused | ||
| let (unresolved_mark, top_level_mark) = resolve_module_mark( | ||
| &mut script.ast, | ||
| cached_module.module.module_type.is_typescript(), | ||
| context.meta.get_globals(&cached_module.module.id).value(), | ||
| ); | ||
|
|
||
| script.top_level_mark = unresolved_mark.as_u32(); | ||
| script.unresolved_mark = top_level_mark.as_u32(); |
There was a problem hiding this comment.
Potential mark usage reversal in handle_cached_modules.
The function returns (unresolved_mark, top_level_mark), then lines 272–273 assign them reversed:
script.top_level_mark = unresolved_mark.as_u32();
script.unresolved_mark = top_level_mark.as_u32();
Double-check the intended usage. If the function name indicates the first value is the “unresolved” mark, you might inadvertently swap the roles here.
| let hook_context = PluginHookContext::default(); | ||
| let res = context | ||
| .plugin_driver | ||
| .render_resource_pot(resource_pot, context, &hook_context)? | ||
| .ok_or(CompilationError::GenerateResourcesError { | ||
| name: resource_pot.id.clone(), | ||
| ty: resource_pot.resource_pot_type.clone(), | ||
| source: None, | ||
| })?; | ||
| resource_pot.meta = res; | ||
| let (mut updated_result, _) = | ||
| render_resource_pot_generate_resources(resource_pot, context, &Default::default())?; | ||
| let update_resources = updated_result.resources.remove(0); | ||
|
|
||
| let sourcemap = update_resources | ||
| .source_map | ||
| .map(|map| SourceMap::from_slice(&map.bytes).unwrap()); | ||
|
|
||
| let code = String::from_utf8(update_resources.resource.bytes).unwrap(); | ||
| let global_this = get_farm_global_this( | ||
| &context.config.runtime.namespace, | ||
| &context.config.output.target_env, | ||
| ); | ||
| let mut magic_string = MagicString::new(&code, None); | ||
|
|
||
| // force re-register the affected modules when hmr | ||
| magic_string.prepend(&format!("{global_this}.{FARM_MODULE_SYSTEM}._rg=true;")); | ||
| magic_string.append(&format!("{global_this}.{FARM_MODULE_SYSTEM}._rg=false;")); | ||
|
|
||
| let code = magic_string.to_string(); | ||
|
|
||
| let map = if let Some(sourcemap) = sourcemap { | ||
| // the updated code will be executed in the browser using new Function(code), so we need to | ||
| // add extra (function anonymous() {})() to wrap the code to make sure the sourcemap works as expected | ||
| magic_string.prepend("(function anonymous(\n) {\n"); | ||
| magic_string.append("\n})"); | ||
|
|
||
| let map = magic_string | ||
| .generate_map(SourceMapOptions::default()) | ||
| .unwrap(); | ||
| let sourcemap = | ||
| collapse_sourcemap_chain(vec![sourcemap, map], CollapseSourcemapOptions::default()); | ||
|
|
||
| let mut buf = vec![]; | ||
| sourcemap.to_writer(&mut buf).unwrap(); | ||
| let map_code = String::from_utf8(buf).unwrap(); | ||
|
|
||
| // inline source map | ||
| format!( | ||
| "\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,{}", | ||
| base64_encode(map_code.as_bytes()) | ||
| ) | ||
| } else { | ||
| "".to_string() | ||
| }; | ||
|
|
||
| Ok(format!("{}{}", code, map)) | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential errors more gracefully.
Several .unwrap() calls (e.g., String::from_utf8(...), SourceMap::from_slice(&map.bytes).unwrap()) can panic if the data is malformed. Consider returning an error instead of panicking to keep the compiler robust.
- let code = String::from_utf8(update_resources.resource.bytes).unwrap();
+ let code = String::from_utf8(update_resources.resource.bytes)
+ .map_err(|e| CompilationError::GenericError(format!("UTF-8 decoding issue: {}", e)))?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let hook_context = PluginHookContext::default(); | |
| let res = context | |
| .plugin_driver | |
| .render_resource_pot(resource_pot, context, &hook_context)? | |
| .ok_or(CompilationError::GenerateResourcesError { | |
| name: resource_pot.id.clone(), | |
| ty: resource_pot.resource_pot_type.clone(), | |
| source: None, | |
| })?; | |
| resource_pot.meta = res; | |
| let (mut updated_result, _) = | |
| render_resource_pot_generate_resources(resource_pot, context, &Default::default())?; | |
| let update_resources = updated_result.resources.remove(0); | |
| let sourcemap = update_resources | |
| .source_map | |
| .map(|map| SourceMap::from_slice(&map.bytes).unwrap()); | |
| let code = String::from_utf8(update_resources.resource.bytes).unwrap(); | |
| let global_this = get_farm_global_this( | |
| &context.config.runtime.namespace, | |
| &context.config.output.target_env, | |
| ); | |
| let mut magic_string = MagicString::new(&code, None); | |
| // force re-register the affected modules when hmr | |
| magic_string.prepend(&format!("{global_this}.{FARM_MODULE_SYSTEM}._rg=true;")); | |
| magic_string.append(&format!("{global_this}.{FARM_MODULE_SYSTEM}._rg=false;")); | |
| let code = magic_string.to_string(); | |
| let map = if let Some(sourcemap) = sourcemap { | |
| // the updated code will be executed in the browser using new Function(code), so we need to | |
| // add extra (function anonymous() {})() to wrap the code to make sure the sourcemap works as expected | |
| magic_string.prepend("(function anonymous(\n) {\n"); | |
| magic_string.append("\n})"); | |
| let map = magic_string | |
| .generate_map(SourceMapOptions::default()) | |
| .unwrap(); | |
| let sourcemap = | |
| collapse_sourcemap_chain(vec![sourcemap, map], CollapseSourcemapOptions::default()); | |
| let mut buf = vec![]; | |
| sourcemap.to_writer(&mut buf).unwrap(); | |
| let map_code = String::from_utf8(buf).unwrap(); | |
| // inline source map | |
| format!( | |
| "\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,{}", | |
| base64_encode(map_code.as_bytes()) | |
| ) | |
| } else { | |
| "".to_string() | |
| }; | |
| Ok(format!("{}{}", code, map)) | |
| }; | |
| let hook_context = PluginHookContext::default(); | |
| let res = context | |
| .plugin_driver | |
| .render_resource_pot(resource_pot, context, &hook_context)? | |
| .ok_or(CompilationError::GenerateResourcesError { | |
| name: resource_pot.id.clone(), | |
| ty: resource_pot.resource_pot_type.clone(), | |
| source: None, | |
| })?; | |
| resource_pot.meta = res; | |
| let (mut updated_result, _) = | |
| render_resource_pot_generate_resources(resource_pot, context, &Default::default())?; | |
| let update_resources = updated_result.resources.remove(0); | |
| let sourcemap = update_resources | |
| .source_map | |
| .map(|map| SourceMap::from_slice(&map.bytes).unwrap()); | |
| let code = String::from_utf8(update_resources.resource.bytes) | |
| .map_err(|e| CompilationError::GenericError(format!("UTF-8 decoding issue: {}", e)))?; | |
| let global_this = get_farm_global_this( | |
| &context.config.runtime.namespace, | |
| &context.config.output.target_env, | |
| ); | |
| let mut magic_string = MagicString::new(&code, None); | |
| // force re-register the affected modules when hmr | |
| magic_string.prepend(&format!("{global_this}.{FARM_MODULE_SYSTEM}._rg=true;")); | |
| magic_string.append(&format!("{global_this}.{FARM_MODULE_SYSTEM}._rg=false;")); | |
| let code = magic_string.to_string(); | |
| let map = if let Some(sourcemap) = sourcemap { | |
| // the updated code will be executed in the browser using new Function(code), so we need to | |
| // add extra (function anonymous() {})() to wrap the code to make sure the sourcemap works as expected | |
| magic_string.prepend("(function anonymous(\n) {\n"); | |
| magic_string.append("\n})"); | |
| let map = magic_string | |
| .generate_map(SourceMapOptions::default()) | |
| .unwrap(); | |
| let sourcemap = | |
| collapse_sourcemap_chain(vec![sourcemap, map], CollapseSourcemapOptions::default()); | |
| let mut buf = vec![]; | |
| sourcemap.to_writer(&mut buf).unwrap(); | |
| let map_code = String::from_utf8(buf).unwrap(); | |
| // inline source map | |
| format!( | |
| "\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,{}", | |
| base64_encode(map_code.as_bytes()) | |
| ) | |
| } else { | |
| "".to_string() | |
| }; | |
| Ok(format!("{}{}", code, map)) | |
| }; |
* feat: add js plugin freeze_modules hooks * chore: remove unless code * chore: update code * chore: remove format * chore: update code * chore: update types * chore: update code * chore: update code * chore: update code * chore: update code * chore: add module_hook_common
* chore: write generate resource moves into rust side * chore: update generate resource write * feat: add rust plugin file size * chore: update file-size plugins * chore: update file-size plugins * chore: update code * chore: update code * chore: rewrite writeResourceToDisk * chore: remove unless file * chore: remove file size in farm crate * chore: remove file size in farm crate * chore: remove file size in farm crate * chore: add schema
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
crates/compiler/src/lib.rs (1)
33-33: Exposelast_fail_module_idscarefully.
Making thelast_fail_module_idsfield publicly accessible might allow inconsistent mutation. Consider restricting visibility to preserve thread safety and provide controlled access.crates/compiler/src/write/mod.rs (6)
1-15: Consider makingSMALL_FILE_THRESHOLDconfigurable.
Currently hard-coded at 8192 bytes. The TODO indicates possible platform-specific values. Exposing it in config might improve flexibility.
16-27:pub(crate) fn writewith TODO for plugin hooks.
Leaving a placeholder for write-bundle hooks is fine. Ensure any plugin hooks are integrated before merging to keep extension points consistent.
53-77: Handling public directory copying.
Implementation is straightforward, with a warning if output and public dirs are the same. Consider adding logs that use a consistent logging approach instead ofprintln!.
95-116: Recursive directory copy with.unwrap().
Usingunwrap()for I/O operations can cause a panic if an error occurs. It's safer to propagate errors (?) or handle them gracefully.fn copy_dir(&self, from: &Path, to: &Path) { if !from.exists() { return; } if let Err(e) = create_dir_all(to) { - create_dir_all(to).unwrap(); + eprintln!("Failed to create directory {}: {e}", to.display()); + return; } // ... }
118-137:create_output_directorieswith potential error handling.
Currently usesunwrap()forcreate_dir_all. Consider logging or returningErr(...)to handle permissions or I/O failures.
178-188: Sequential writing of small files.
Same concern as with large files: error handling can be improved by returning aResultfrom each write rather than ignoring potential failures.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
crates/compiler/Cargo.toml(2 hunks)crates/compiler/src/lib.rs(8 hunks)crates/compiler/src/write/mod.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/compiler/Cargo.toml
🔇 Additional comments (16)
crates/compiler/src/lib.rs (13)
15-16: Imports look good.
No immediate issues with addingModuleIdandparking_lot::Mutexhere.
22-22: Public re-export ofRUNTIME_INPUT_SCOPE.
This neatly centralizes usage of the runtime scope across the codebase.
28-29: New modulesutilsandwrite.
Creating separate modules for utility and write operations improves maintainability.
39-43: Conditional plugin assignment based on target environment.
The approach properly differentiates between library and runtime targets. No concerns.
46-49: Initialize script meta features and render plugin.
Plugin ordering logic appears consistent with the design requirements. Looks good.
62-64: AddFarmPluginScriptMeta.
Adding script meta plugin is straightforward. Implementation details look fine.
81-86: InsertFarmPluginScriptMetaExports.
Placing this plugin post-tree-shaking and pre-mangling is logical. No issues found.
92-100: Conditionally apply mangle exports.
The logic correctly checksoptions.mangle_exportsbefore loaded. Implementation is clear.
110-112: Show file size plugin for production mode.
This conditional logic is aligned with typical usage. Fine as is.
158-158: Record start time in compile method.
No functional concerns. Stats usage is consistent.
171-173: Set build end time after build stage.
Nicely placed to capture build duration accurately.
180-184: Profile scope for write stage.
Callingself.write()after generation is a sensible flow.
205-205: Set final end time in compile.
Closes out the compile stats properly.crates/compiler/src/write/mod.rs (3)
29-51:write_resources_to_diskmethod structure.
Overall logic is clear: lock resources, split them, write, and copy public dir. Good approach for staged writing.
79-93: Directory overlap check.
The subpath check for separate folders can be fragile if symbolic links or unusual paths exist. Consider verifying symlink scenarios if relevant to your environment.
139-162: Resource splitting by size.
Splitting small and large files is straightforward. Just ensure that any custom threshold logic covers edge cases for extremely large files or very small resources.
| fn write_large_files( | ||
| &self, | ||
| large_files: &[(&String, &Resource)], | ||
| output_dir: &Path, | ||
| ) -> Result<()> { | ||
| // temporary use try_for_each to avoid panic and ok return instead of Result | ||
| large_files.par_iter().for_each(|&(name, resource)| { | ||
| let path = output_dir.join(name.split(['?', '#']).next().unwrap()); | ||
| self.write_file(&path, &resource.bytes); | ||
| }); | ||
|
|
||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parallel writing of large files.
The try_for_each style loop does not propagate I/O errors. If a single file fails to write, the entire process might appear successful. Consider capturing errors in the iteration.
| // TODO use error::{CompilationError} we need refactor Error mod | ||
| fn write_file(&self, path: &Path, content: &[u8]) { | ||
| let file = File::create(path).unwrap(); | ||
| let mut writer = BufWriter::new(file); | ||
| writer.write_all(content).unwrap(); | ||
| writer.flush().unwrap(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
write_file uses .unwrap().
Repeated .unwrap() calls can cause abrupt panics. Returning errors or logging them is typically safer for a compiler system.
Description:
BREAKING CHANGE:
Related issue (if exists):
#1589
Summary by CodeRabbit
__commonJsfunction for streamlined module handling.