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>
WalkthroughThis pull request introduces numerous updates across configuration files, CI workflows, core compiler modules, and tests. New changeset entries document major dependency version updates and API exports. The GitHub Actions workflows now support the new branch v2-dev and use an updated pnpm version. Inside the compiler, several internal functions have been refactored for building, updating, and writing resources. Benchmark files are removed, and many tests have been added or updated to verify module‐graph patching, dynamic entries, and export declarations. A new resource writing module and several module system initialization functions have also been integrated. Changes
Sequence Diagram(s)sequenceDiagram
participant C as Compiler.write()
participant W as write_resources_to_disk()
participant D as create_output_directories()
participant S as split_resources()
participant L as write_large_files()/write_small_files()
participant F as write_file()
C->>W: Invoke write_resources_to_disk()
W->>D: Create output directories
W->>S: Split resources (small vs large)
S->>L: Dispatch file-writing tasks
L->>F: Write individual files
F-->>L: File written
L-->>W: Completing resource write
W-->>C: Writing process finished
Poem
✨ 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.
| // 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.
* fix: module concatenation issues * fix: ci issues
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
crates/compiler/tests/fixtures/library/external/conflicts/output.js (1)
1-16:⚠️ Potential issuePotential reference errors in generated output
There are two undefined references in this generated output file that could cause runtime errors:
node_module_external_all_farm_internal_on line 9 is not defined anywherecreateRequireon line 15 is not imported or definedThis appears to be generated code, but you should verify these references are properly defined before they're used.
#!/bin/bash # Check if these identifiers are defined elsewhere in the codebase rg "node_module_external_all_farm_internal_" --type js rg "createRequire" --type js
♻️ Duplicate comments (2)
crates/compiler/src/update/regenerate_resources/mod.rs (1)
82-143:⚠️ Potential issueHandle potential errors more gracefully in resource processing.
Several
.unwrap()calls could panic if the data is malformed. Consider returning errors instead of panicking to maintain compiler robustness, especially for user-provided resources.Apply these changes:
- let sourcemap = update_resources - .source_map - .map(|map| SourceMap::from_slice(&map.bytes).unwrap()); + let sourcemap = update_resources + .source_map + .map(|map| SourceMap::from_slice(&map.bytes) + .map_err(|e| CompilationError::GenerateResourcesError { + name: resource_pot.id.clone(), + ty: resource_pot.resource_pot_type.clone(), + source: Some(format!("Invalid source map: {}", e)), + })?); - let code = String::from_utf8(update_resources.resource.bytes).unwrap(); + let code = String::from_utf8(update_resources.resource.bytes) + .map_err(|e| CompilationError::GenerateResourcesError { + name: resource_pot.id.clone(), + ty: resource_pot.resource_pot_type.clone(), + source: Some(format!("UTF-8 decoding issue: {}", e)), + })?; - sourcemap.to_writer(&mut buf).unwrap(); - let map_code = String::from_utf8(buf).unwrap(); + sourcemap.to_writer(&mut buf) + .map_err(|e| CompilationError::GenerateResourcesError { + name: resource_pot.id.clone(), + ty: resource_pot.resource_pot_type.clone(), + source: Some(format!("Failed to write source map: {}", e)), + })?; + let map_code = String::from_utf8(buf) + .map_err(|e| CompilationError::GenerateResourcesError { + name: resource_pot.id.clone(), + ty: resource_pot.resource_pot_type.clone(), + source: Some(format!("Invalid UTF-8 in source map: {}", e)), + })?;crates/compiler/src/write/mod.rs (1)
34-58: 🛠️ Refactor suggestion
write_resources_to_disk()Method
- Gating profiling calls with
#[cfg(feature = "profile")]is good for optional overhead.- Using
let resources_map = self.context.resources_map.lock();ensures exclusive access. The subsequent steps (creating directories, splitting, writing large/small files) look logically consistent.- The method uses
.unwrap()oncreate_output_directories(...),write_large_files(...), andwrite_small_files(...). While this ensures the pipeline stops on error, you might consider capturing errors or returning aResultto unify error handling.- .unwrap(); + ?;Adjusting these calls to propagate errors rather than unwrapping unexpectedly can improve reliability.
🧹 Nitpick comments (28)
crates/compiler/tests/fixtures/library/decl/self_ref/output.js (1)
3-3: Consider using unique console messages for easier debuggingAll console log messages use the same string 'addClass', which could make debugging challenging when trying to trace which component is logging. Consider using more specific messages that identify the exact source.
function addClass() { - console.log('addClass'); + console.log('addClass function called'); } var exportFn_ts_default = addClass; class AddClass { constructor(){ - console.log('addClass'); + console.log('AddClass constructor called'); } } var exportClass_ts_default = AddClass; var _addClass = function addClass() { - console.log('addClass'); + console.log('_addClass function called'); exportFn_ts_default(); }; var _AddClass = class AddClass { constructor(){ - console.log('addClass'); + console.log('_AddClass constructor called'); new exportClass_ts_default(); } };Also applies to: 8-8, 13-13, 18-18
TODO.txt (2)
3-3: Pending Task Reminder – Wildcard Export IssueThe task “[] fix concatenate module issues for
export * from 'fs';” is still pending. Please update this when you have a fix in place or create an issue detailing the work needed. Additional context could help streamline the resolution.
4-4: NIT: OS Name CapitalizationThe task “[] test arco stuck on macos and windows” uses “macos” in lowercase. For consistency with conventional OS naming (as also noted in the static analysis hints), update it to “macOS”. Consider also verifying that “Windows” is appropriately capitalized if needed.
Below is a diff suggestion for clarity:
- [] test arco stuck on macos and windows + [] test arco stuck on macOS and Windows🧰 Tools
🪛 LanguageTool
[grammar] ~4-~4: The operating system from Apple is written “macOS”.
Context: ...ort * from 'fs';` [] test arco stuck on macos and windows(MAC_OS)
crates/compiler/tests/fixtures/external/node/hosting/import/output.js (1)
2-6: Simplified module system initialization approachThe new implementation uses Node.js's native
createRequirefunction to create a dynamic require function, which is a more standard approach than the previous custom interop helper functions. This aligns with modern JavaScript module handling practices.However, the
initModuleSystemfunction doesn't appear to be using themoduleSystemparameter passed to it on line 6, which could be a potential oversight.Consider updating to ensure the parameter is properly utilized:
-function initModuleSystem() { +function initModuleSystem(moduleSystem) { console.log('module-helper.ts'); // Use moduleSystem parameter here }crates/compiler/tests/fixtures/external/browser/object/esm/output.js (2)
7-9: Consider adding error handling for external dependenciesThe code assumes
window['__farm_default_namespace__']and its methods exist without verification. If these are not properly initialized elsewhere, this could lead to runtime errors.-}());window['__farm_default_namespace__'].m.se({ +}()); + +// Set external modules +if (window['__farm_default_namespace__'] && window['__farm_default_namespace__'].m && + typeof window['__farm_default_namespace__'].m.se === 'function') { + window['__farm_default_namespace__'].m.se({ "jquery": window['$'] || {} -}); + }); +} else { + console.error('Farm module system not properly initialized'); +}
22-22: Improve code readability by separating statementsMultiple operations are combined in a single line, which reduces readability and can cause issues with some minifiers or linters.
-var __farm_ms__ = window['__farm_default_namespace__'].m;__farm_ms__.b();var __farm_entry__=__farm_ms__.r("b5d64806"); +var __farm_ms__ = window['__farm_default_namespace__'].m; +__farm_ms__.b(); +var __farm_entry__ = __farm_ms__.r("b5d64806");crates/compiler/tests/fixtures/external/browser/object/cjs/output.js (2)
7-9: Consider adding error handling for external dependenciesThe code assumes
window['__farm_default_namespace__']and its methods exist without verification. If these are not properly initialized elsewhere, this could lead to runtime errors.-}());window['__farm_default_namespace__'].m.se({ +}()); + +// Set external modules +if (window['__farm_default_namespace__'] && window['__farm_default_namespace__'].m && + typeof window['__farm_default_namespace__'].m.se === 'function') { + window['__farm_default_namespace__'].m.se({ "jquery": window['$'] || {} -}); + }); +} else { + console.error('Farm module system not properly initialized'); +}
22-22: Improve code readability by separating statementsMultiple operations are combined in a single line, which reduces readability and can cause issues with some minifiers or linters.
-var __farm_ms__ = window['__farm_default_namespace__'].m;__farm_ms__.b();var __farm_entry__=__farm_ms__.r("b5d64806"); +var __farm_ms__ = window['__farm_default_namespace__'].m; +__farm_ms__.b(); +var __farm_entry__ = __farm_ms__.r("b5d64806");crates/compiler/tests/fixtures/external/browser/normal/cjs/output.js (2)
7-9: Consider adding error handling for external dependenciesThe code assumes
window['__farm_default_namespace__']and its methods exist without verification. If these are not properly initialized elsewhere, this could lead to runtime errors.-}());window['__farm_default_namespace__'].m.se({ +}()); + +// Set external modules +if (window['__farm_default_namespace__'] && window['__farm_default_namespace__'].m && + typeof window['__farm_default_namespace__'].m.se === 'function') { + window['__farm_default_namespace__'].m.se({ "jquery": window['jquery'] || {} -}); + }); +} else { + console.error('Farm module system not properly initialized'); +}
22-22: Improve code readability by separating statementsMultiple operations are combined in a single line, which reduces readability and can cause issues with some minifiers or linters.
-var __farm_ms__ = window['__farm_default_namespace__'].m;__farm_ms__.b();var __farm_entry__=__farm_ms__.r("b5d64806"); +var __farm_ms__ = window['__farm_default_namespace__'].m; +__farm_ms__.b(); +var __farm_entry__ = __farm_ms__.r("b5d64806");crates/compiler/tests/fixtures/external/node/normala/cjs/output.js (2)
7-10: Add error handling for external module loadingThe code directly requires jQuery without checking for its existence or handling potential errors if the module is not installed.
-}());var __farm_external_module_jquery = require("jquery"); -global['__farm_default_namespace__'].m.se({ - "jquery": __farm_external_module_jquery -}); +}()); + +// Load external modules +let __farm_external_module_jquery; +try { + __farm_external_module_jquery = require("jquery"); +} catch (e) { + console.error('Failed to load jQuery:', e.message); + __farm_external_module_jquery = {}; +} + +// Set external modules +if (global['__farm_default_namespace__'] && global['__farm_default_namespace__'].m && + typeof global['__farm_default_namespace__'].m.se === 'function') { + global['__farm_default_namespace__'].m.se({ + "jquery": __farm_external_module_jquery + }); +} else { + console.error('Farm module system not properly initialized'); +}
23-23: Improve code readability by separating statementsMultiple operations are combined in a single line, which reduces readability and can cause issues with some minifiers or linters.
-var __farm_ms__ = global['__farm_default_namespace__'].m;__farm_ms__.b();var __farm_entry__=__farm_ms__.r("b5d64806"); +var __farm_ms__ = global['__farm_default_namespace__'].m; +__farm_ms__.b(); +var __farm_entry__ = __farm_ms__.r("b5d64806");crates/compiler/tests/fixtures/external/browser/normal/esm/output.js (1)
2-6: Module system initialization has been completely refactoredThe code now uses a much simpler approach to initialize the module system. The previous interop helper functions (
_interop_require_default,_export_star, etc.) have been removed in favor of this streamlined IIFE pattern.For improved debuggability, consider adding a more descriptive log message in
initModuleSystemthat includes version information:function initModuleSystem() { - console.log('module-helper.ts'); + console.log('[Farm v2.0] Initializing module system'); }crates/compiler/tests/bundle.rs (2)
76-77: Default assignment pattern updateUsing Default::default() for remaining config values is a more idiomatic Rust pattern.
100-108: Commented-out test casesThese commented test cases might be intended for future use, but consider either:
- Removing them if they're no longer needed
- Adding a TODO comment explaining why they're commented out and when they might be re-enabled
Commented-out code can confuse future contributors about whether these tests are failing, deprecated, or pending implementation.
crates/compiler/src/generate/partial_bundling.rs (1)
256-283:dynamic_entry_module_group_to_resource_pot
Implementation is consistent with the rest of the bundling logic. Consider whether any shared logic with enforce resource pots can be factored out if duplication becomes extensive.crates/compiler/tests/fixtures/library/farm_runtime/modules/module-helper.ts (1)
1-5: ExportedinitModuleSystem
This method logs "module-system" when called. Consider typing themsparameter if using TypeScript to enhance maintainability.-export function initModuleSystem(ms) { +export function initModuleSystem(ms: { _m?: () => void }) { ms._m = function() { console.log('module-system'); } }crates/compiler/tests/fixtures/library/farm_runtime/modules/plugin.ts (1)
3-7: LGTM, but consider adding documentationThe implementation of the
initModuleSystemfunction looks good. It correctly adds a plugin method to the module system object.Consider adding JSDoc comments to explain the purpose of this function and the expected shape of the
msparameter:+/** + * Initializes the module system with plugin functionality + * @param {Object} ms - The module system object to extend + */ export function initModuleSystem(ms) { ms.p = function() { console.log('plugin'); } }crates/compiler/src/generate/render_resource_pots.rs (2)
29-54: Potential lock management issue in resource handling.When handling cached resources, you're accessing the mutex-protected
resourcesmultiple times without releasing the lock between operations. Consider using a scoped approach to avoid potential deadlocks and improve mutex usage.-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); - - if let Some(map) = cached_resource.source_map { - resource_pot.add_resource(map.name.clone()); - - resources.lock().push(map); - } -} +// First, add all resource names to the pot +for cached_resource in &cached_resources.resources { + resource_pot.add_resource(cached_resource.resource.name.clone()); + if let Some(map) = &cached_resource.source_map { + resource_pot.add_resource(map.name.clone()); + } +} + +// Then, with a single lock, push all resources +{ + let mut locked_resources = resources.lock(); + for cached_resource in cached_resources.resources { + locked_resources.push(cached_resource.resource); + if let Some(map) = cached_resource.source_map { + locked_resources.push(map); + } + } +}
129-165: Consider moving cache check outside conditional block.The persistent cache check is inside the source map handling block, but it might be clearer to separate the caching logic from the source map handling to improve code organization.
-if let Some(mut source_map) = res.source_map { - source_map.name = format!( - "{}.{}", - res.resource.name, - source_map.resource_type.to_ext() - ); - append_sourcemap_comment(&mut res.resource, &source_map, &context.config.sourcemap); - - if context.config.persistent_cache.enabled() { - cached_resource.source_map = Some(source_map.clone()); - } - - resource_pot.add_resource(source_map.name.clone()); - - resources.lock().push(source_map); -} +if let Some(mut source_map) = res.source_map { + source_map.name = format!( + "{}.{}", + res.resource.name, + source_map.resource_type.to_ext() + ); + append_sourcemap_comment(&mut res.resource, &source_map, &context.config.sourcemap); + + resource_pot.add_resource(source_map.name.clone()); + resources.lock().push(source_map.clone()); + + if context.config.persistent_cache.enabled() { + cached_resource.source_map = Some(source_map); + } +}crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (1)
142-142: Ensure deterministic sorting for tests.When sorting
resource_pot_ids, it would be more explicit to specify the sorting criterion to ensure deterministic test behavior, especially if the order matters for snapshots.-resource_pot_ids.sort(); +resource_pot_ids.sort_by(|a, b| a.cmp(b));crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (1)
131-145: Extract empty resource pot removal logic for better modularity.The new function
remove_empty_resource_potsencapsulates the logic for removing resource pots without modules, improving code readability and maintenance. However, it uses a panic for error handling, which could be improved.Consider replacing the panic with Result-based error handling for more graceful recovery:
- let resource_pot = resource_pot_map.resource_pot_mut(id).unwrap_or_else(|| { - panic!("resource pot not found: {id:?}"); - }); + let resource_pot = match resource_pot_map.resource_pot_mut(id) { + Some(pot) => pot, + None => { + log::warn!("Resource pot not found: {id:?}, skipping removal"); + continue; + } + };crates/compiler/tests/fixtures/css/modules/local/output.js (1)
3-4: Clarify the console message
console.log('module-helper.ts');references a TypeScript filename from within a JavaScript file. If this is just a placeholder log, consider renaming it to reduce confusion, such as"Initializing module system...".- console.log('module-helper.ts'); + console.log('Initializing module system...');crates/compiler/src/write/mod.rs (5)
12-15: SplitResources AliasDefines a tuple for small and large resources. Straightforward, but consider documenting each tuple element to enhance readability for future maintainers.
21-22: Refactoring the Error ModuleA TODO indicates that a dedicated error type might be introduced. Since error handling is crucial for compilers, addressing this soon will improve maintainability.
Would you like help drafting a custom error type for these operations?
102-123:copy_dir()Method
- Uses a basic loop over
read_dir(from).unwrap(). The.unwrap()is acceptable in many scenarios but can lead to abrupt termination if the source directory fails to read.- Consider returning a
Resultfrom this function for safer error handling.
168-180:write_large_files()Method
- Utilizes parallel iteration via
large_files.par_iter().- The inline closure calls
self.write_file(...). Currently the function discards potential I/O errors, but you do return anOk(()).- Consider capturing or short-circuiting on error.
194-200:write_file()Method
- Straight-line writes with
BufWriter, followed by.flush()..unwrap()usage could lead to panic; returning or logging the error would be safer.- The code is otherwise clear and well-structured for writing file contents.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (20)
crates/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!**/*.snapcrates/compiler/tests/fixtures/library/export/exportDefaultDecl/node_modules/@swc/helpers/_/_interop_require_default/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (72)
TODO.txt(1 hunks)biome.json(5 hunks)crates/compiler/src/generate/partial_bundling.rs(9 hunks)crates/compiler/src/generate/render_resource_pots.rs(4 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/test_remove_and_add.rs(1 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(9 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/mod.rs(6 hunks)crates/compiler/src/write/mod.rs(1 hunks)crates/compiler/tests/bundle.rs(5 hunks)crates/compiler/tests/fixtures/_internal/runtime/src/modules/plugin.ts(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(2 hunks)crates/compiler/tests/fixtures/css/modules/local/output.js(2 hunks)crates/compiler/tests/fixtures/css/modules/normal/output.js(2 hunks)crates/compiler/tests/fixtures/css/prefixer/normal/output.js(2 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/browser/external/output.js(1 hunks)crates/compiler/tests/fixtures/library/browser/normal/output.js(1 hunks)crates/compiler/tests/fixtures/library/decl/export_default_decl/no-ident/output.js(1 hunks)crates/compiler/tests/fixtures/library/decl/normal/output.js(1 hunks)crates/compiler/tests/fixtures/library/decl/self_ref/output.js(1 hunks)crates/compiler/tests/fixtures/library/decl/spread/output.js(1 hunks)crates/compiler/tests/fixtures/library/decl/unresolved_variable/output.js(1 hunks)crates/compiler/tests/fixtures/library/decl/var/output.js(1 hunks)crates/compiler/tests/fixtures/library/dynamic/normal/index.ts(1 hunks)crates/compiler/tests/fixtures/library/dynamic/normal/output.js(1 hunks)crates/compiler/tests/fixtures/library/dynamic/normal/same-name/dynamic.ts(1 hunks)crates/compiler/tests/fixtures/library/export/emptyExport/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/entryExport1/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/entryExport2/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/entryExport3/index.ts(1 hunks)crates/compiler/tests/fixtures/library/export/entryExport3/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/exportDefault1/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/exportDefaultDecl/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/exportDefaultExpr/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/exportNamed1/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/exportNamed2/output.js(1 hunks)crates/compiler/tests/fixtures/library/export/exportNamed3/index.ts(1 hunks)crates/compiler/tests/fixtures/library/export/exportNamed3/output.js(1 hunks)crates/compiler/tests/fixtures/library/external/conflicts/bar.ts(1 hunks)crates/compiler/tests/fixtures/library/external/conflicts/foo.ts(1 hunks)crates/compiler/tests/fixtures/library/external/conflicts/index.ts(1 hunks)crates/compiler/tests/fixtures/library/external/conflicts/output.js(1 hunks)crates/compiler/tests/fixtures/library/external/conflicts/zoo.ts(1 hunks)crates/compiler/tests/fixtures/library/external/export_all/bar.ts(1 hunks)crates/compiler/tests/fixtures/library/external/export_all/index.ts(1 hunks)crates/compiler/tests/fixtures/library/external/export_all/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/farm_runtime/index.ts(1 hunks)crates/compiler/tests/fixtures/library/farm_runtime/module-system.ts(1 hunks)crates/compiler/tests/fixtures/library/farm_runtime/modules/module-helper.ts(1 hunks)crates/compiler/tests/fixtures/library/farm_runtime/modules/plugin.ts(1 hunks)
✅ Files skipped from review due to trivial changes (28)
- crates/compiler/tests/fixtures/library/farm_runtime/module-system.ts
- crates/compiler/tests/fixtures/library/external/conflicts/foo.ts
- crates/compiler/tests/fixtures/library/dynamic/normal/index.ts
- crates/compiler/tests/fixtures/library/external/import_namespace/config.json
- crates/compiler/tests/fixtures/library/export/emptyExport/output.js
- crates/compiler/tests/fixtures/library/external/export_all/index.ts
- crates/compiler/tests/fixtures/library/external/conflicts/bar.ts
- crates/compiler/tests/fixtures/library/decl/export_default_decl/no-ident/output.js
- crates/compiler/tests/fixtures/library/export/exportDefaultDecl/output.js
- crates/compiler/tests/fixtures/library/decl/var/output.js
- crates/compiler/tests/fixtures/library/export/exportNamed2/output.js
- crates/compiler/tests/fixtures/library/external/conflicts/zoo.ts
- crates/compiler/tests/fixtures/library/external/export_all/output.js
- crates/compiler/tests/fixtures/library/farm_runtime/index.ts
- crates/compiler/tests/fixtures/library/external/import_named/output.js
- crates/compiler/tests/fixtures/library/decl/unresolved_variable/output.js
- crates/compiler/tests/fixtures/library/external/export_all/bar.ts
- crates/compiler/tests/fixtures/library/external/import_named/index.ts
- crates/compiler/tests/fixtures/library/export/entryExport3/index.ts
- crates/compiler/tests/fixtures/library/export/exportDefault1/output.js
- crates/compiler/tests/fixtures/library/external/conflicts/index.ts
- crates/compiler/tests/fixtures/library/external/import_namespace/output.js
- crates/compiler/tests/fixtures/library/export/exportDefaultExpr/output.js
- crates/compiler/tests/fixtures/library/export/exportNamed3/output.js
- crates/compiler/tests/fixtures/library/external/import_namespace/config.cjs.json
- crates/compiler/tests/fixtures/library/external/import_namespace/output.cjs.js
- crates/compiler/tests/fixtures/library/decl/spread/output.js
- crates/compiler/tests/fixtures/library/export/exportNamed1/output.js
🚧 Files skipped from review as they are similar to previous changes (2)
- biome.json
- crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_dynamic_entry_resource_pots.rs
🧰 Additional context used
🧬 Code Definitions (20)
crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/browser/object/cjs/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (4)
crates/compiler/src/update/diff_and_patch_module_graph.rs (2)
diff_module_graph(91-128)patch_module_graph(131-205)crates/compiler/src/update/patch_module_group_graph.rs (3)
patch_module_group_graph(15-86)module_graph(580-584)module_graph(627-631)crates/compiler/src/update/regenerate_resources/mod.rs (2)
clear_resource_pot_of_modules_in_module_groups(234-249)resource_pot_map(205-209)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (2)
generate_and_diff_resource_pots(34-83)resource_pots(268-271)
crates/compiler/tests/fixtures/external/node/normala/cjs/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/node/hosting/import/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/node/hosting/require/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/modules/local/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/modules/global/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/browser/object/esm/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/node/normala/esm/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/src/generate/render_resource_pots.rs (5)
crates/node/src/lib.rs (3)
resource(470-475)resources(410-424)resources_map(427-437)crates/plugin_css/src/lib.rs (4)
resource_pot(647-651)context(508-520)name(80-82)name(186-188)crates/plugin_script/src/lib.rs (2)
resource_pot(435-439)name(64-66)crates/toolkit/src/sourcemap.rs (1)
append_sourcemap_comment(35-68)crates/compiler/src/lib.rs (1)
context(210-212)
crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/browser/normal/cjs/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/tests/fixtures/external/browser/normal/esm/output.js (1)
packages/runtime/src/module-system.ts (1)
farmRequire(117-215)
crates/compiler/src/update/patch_module_group_graph.rs (2)
crates/core/src/module/module_graph.rs (2)
module(292-300)edge_info(470-485)crates/core/src/module/module_group.rs (1)
module_group(79-82)
🪛 LanguageTool
TODO.txt
[grammar] ~4-~4: The operating system from Apple is written “macOS”.
Context: ...ort * from 'fs';` [] test arco stuck on macos and windows
(MAC_OS)
🔇 Additional comments (150)
crates/compiler/tests/fixtures/library/export/exportNamed3/index.ts (1)
5-6: Good addition to test browser-specific exports.This code adds a new test case that destructures and exports properties from the global
windowobject. This is a valuable test fixture as it verifies the compiler's ability to handle this specific export pattern correctly.Since this is a test fixture, the browser-specific nature (
windowobject) is appropriate in this context, as it tests how the Farm compiler handles web APIs when they're exported as named exports from modules.crates/compiler/tests/fixtures/library/dynamic/normal/same-name/dynamic.ts (1)
1-3: Looks good! Simple and focused test fixture.The exported function follows good practices with clear naming that matches the file purpose. This test fixture will be useful for testing dynamic imports with same-name modules.
crates/compiler/tests/fixtures/library/dynamic/normal/output.js (3)
1-8: Generated code for dynamic imports looks correct.This section properly defines the
foo$1function and creates a namespace object with the correct ESM compatibility properties. The structure follows standard patterns for handling ES modules in compiled output.
9-15: Successfully implements the importedsameNameDynamicfunction.The function implementation correctly matches the source from
dynamic.tsand is properly wrapped in a namespace object with ESM compatibility.
16-26: Dynamic module loading patterns are properly implemented.The various Promise patterns demonstrate different ways of handling dynamic imports:
- Direct module loading and execution
- Function that returns a Promise-wrapped module
- Loader function pattern
- Object property with Promise-wrapped module
- Block-scoped module resolution
This comprehensive test fixture will effectively validate the Farm compiler's handling of various dynamic import patterns.
crates/compiler/tests/fixtures/library/decl/self_ref/output.js (4)
1-11: Clear module export pattern for functions and classesThis segment establishes a pattern for exporting functions and classes, using the
exportFn_ts_defaultandexportClass_ts_defaultvariables as intermediaries. This approach follows modern JavaScript bundling conventions for default exports.
12-21: Self-referential implementation demonstrates circular dependency handlingThe implementation of
_addClassand_AddClassdemonstrates how to handle self-references by calling back to the exported versions of themselves. This pattern tests the bundler's capability to properly resolve circular references in module systems.
12-15: Potential stack overflow risk with circular function callsThe implementation of
_addClasscreates a potential infinite loop by callingexportFn_ts_default()without any termination condition. While this may be intentional for testing purposes, be aware that this would cause a stack overflow in production.
16-21: Watch for memory leaks with recursive class instantiationThe
_AddClassconstructor creates a new instance ofexportClass_ts_defaulteach time it's instantiated, which could lead to excessive object creation and memory issues if not handled properly. If this is meant for testing circular dependencies, consider adding a comment explaining its purpose.crates/compiler/tests/fixtures/external/node/hosting/require/output.js (5)
2-6: Module system initialization has been significantly simplified.The code now leverages Node.js's
createRequirefunction instead of custom interop utility functions that were previously used. The new approach initializes a module system and sets up the foundation for module loading.
7-10: External module registration looks good.The code properly imports jQuery and registers it in the global namespace under
__farm_default_namespace__, making it available to the module system. This aligns with the__FARM_ENABLE_EXTERNAL_MODULES__feature in the runtime.
11-15: Module registration process is well-structured.This pattern efficiently iterates through the provided modules and registers each one with the module system using the
gmethod, which presumably stands for "register" or "get".
16-22: Module implementation matches runtime expectations.The module implementation correctly uses
farmRequireto import jQuery and follows the function signature pattern established in the runtime's module system. This matches the module function signature inpackages/runtime/src/module-system.tswhere modules receive(module, exports, farmRequire, farmDynamicRequire).
24-24: Entry point initialization looks good.The code properly obtains the module system from the global namespace, initializes it with
.b()(which likely corresponds to "bootstrap"), and then requires the entry module.TODO.txt (2)
1-1: Task Completion Confirmation – ESM MigrationThe task “[x] migrate all esm runtime bundle to library test” is marked as complete. Please ensure through your testing framework that all migrated bundles are verified against the library tests.
2-2: Verification of Fix for Named Export IssueThe task “[x] fix concatenate module issues for
export { readFileSync } from 'fs';readFileSync('111')” is marked complete. Confirm that the fix correctly separates concerns—exporting the function and any test invocation—and that no side effects occur due to the inline call.crates/compiler/tests/fixtures/library/export/entryExport2/output.js (2)
1-19: Test fixture correctly implements module exports patternThis test fixture properly demonstrates the expected output format for compiled module exports, using the Farm internal namespace pattern with
__esModule: trueflag for ES module compatibility.
12-18: Well-structured namespace object for exportsThe namespace object correctly aggregates all exported values and maintains the proper structure with shorthand property notation and the
__esModuleflag.crates/compiler/tests/fixtures/library/export/entryExport1/output.js (1)
1-19: Test fixture correctly implements module exports patternThis test fixture properly demonstrates the expected output format for compiled module exports, using the same Farm internal namespace pattern as seen in entryExport2.
crates/compiler/tests/fixtures/library/export/entryExport3/output.js (3)
1-27: Test fixture correctly handles complex export scenariosThis fixture properly demonstrates multiple export patterns including named exports, re-exports, and namespace exports. The internal namespace structure is consistent with other test fixtures.
5-9: Function includes proper lexical scoping exampleThe
BBfunction correctly demonstrates lexical scoping with local variables shadowing the outer scope, which is good for testing how the compiler handles scope resolution.
24-27: Multiple export statements demonstrate various export patternsThe multiple export statements effectively test different export syntaxes including named exports, namespace exports, and re-exports with aliases.
crates/compiler/tests/fixtures/external/node/hosting/import/output.js (4)
11-15: More straightforward module registration patternThe new module registration pattern is cleaner and follows a more consistent approach by iterating over module definitions and registering them with the module system using the
g()method (likely "get/register").
25-25: Improved entry point bootstrappingThe entry point bootstrapping is now more explicit with the module system being initialized with
__farm_ms__.b()and then requiring the entry module. This makes the execution flow clearer.
7-10:✅ Verification successful
External module handling improved
The external module handling approach is more direct by importing jQuery using ES modules syntax and then registering it with the module system through the
se()method (likely "set external").Verify that this approach works correctly for all external dependencies by running:
🏁 Script executed:
#!/bin/bash # Check how external modules are handled across the codebase rg -A 5 "__farm_external_module_" --type jsLength of output: 4967
External Module Handling Verified
The ES module approach for handling external dependencies is consistent across the codebase. In the file
crates/compiler/tests/fixtures/external/node/hosting/import/output.js, jQuery is now imported using ES modules and then registered with the module system via these()method. The verification output confirms that similar patterns—for instance, for "fs" and "jquery"—are applied in all relevant files, ensuring that the new approach works correctly for all external modules.
16-23:❓ Verification inconclusive
Module import handling simplified
The new module import handling approach is more streamlined. The implementation uses a combination of functions:
farmRequire._m()- Likely marking the exports objectfarmRequire.i()- Likely importing from a modulefarmRequire.f()- Likely formatting or finalizing the importThis approach is more organized than the previous implementation that used multiple interop helper functions.
Verify these utility methods are properly documented:
🏁 Script executed:
#!/bin/bash # Search for documentation of the farmRequire utility methods rg -A 3 "farmRequire\.(i|f|_m)" --type tsLength of output: 825
Utility Methods Documentation Verification
The new module import handling assigns the following utility methods in
packages/runtime/src/modules/module-helper.ts:
farmRequire._mis assigned todefineExportEsModulefarmRequire.iis assigned tointeropRequireDefaultfarmRequire.fis assigned toimportDefaultA search for these assignments (using
rg -A 3 "farmRequire\.(i|f|_m)" --type ts) returned only the code lines without any accompanying inline documentation or JSDoc comments. Please verify that the underlying implementations—namely, the functionsdefineExportEsModule,interopRequireDefault, andimportDefault—are adequately documented either directly in their definitions or in the project’s documentation. If this level of documentation is missing, consider adding descriptive JSDoc comments to clarify the purpose and usage of each utility method.crates/compiler/tests/fixtures/external/browser/normal/esm/output.js (4)
7-9: New external module registration mechanismThis section introduces a new way to register external modules with the Farm module system. It appropriately handles the case where jQuery might not be available in the window object.
10-15: Module registration approach simplifiedThe module registration process now uses a more direct approach with the
moduleSystem.g()method. This aligns with the overall simplification of the module system in Farm v2.0.
22-22:❓ Verification inconclusive
New module bootstrapping mechanism
The bootstrapping process has been refactored to use the module system's
b()method for initialization andr()method to require the entry module.Verify that these methods are correctly implemented in the module system:
🏁 Script executed:
#!/bin/bash # Search for the implementation of the module system's b() and r() methods echo "Searching for implementations of the module system methods..." rg -A 5 "\.b\s*=\s*function|\.r\s*=\s*function" --type ts packages/Length of output: 202
Re: Module Bootstrapping Implementation Verification
The new bootstrapping mechanism appears to use the module system’s
b()andr()methods as intended. However, our initial search for their implementations (using a regex for traditional function syntax) produced no results. This outcome might indicate that the implementations use a different syntax (for example, arrow functions) or reside in unexpected locations.Next Steps:
- Please re-verify that the module system’s
b()andr()methods are correctly implemented. You might want to search using alternative patterns (e.g., for arrow function syntax) or verify their existence manually.- If necessary, update the search pattern to match the current implementation style in the codebase.
16-20:❓ Verification inconclusive
Module definition uses new helper methods
The module definition now uses new helper methods like
farmRequire._m(),farmRequire.i(), andfarmRequire.f(). These appear to be utility functions for module interoperability.Verify that these helper methods are properly documented in the Farm v2.0 documentation, as they represent a breaking change from the previous implementation.
🏁 Script executed:
#!/bin/bash # Search for documentation of the new farmRequire helper methods echo "Searching for documentation of new farmRequire helper methods..." rg -A 3 "farmRequire\._m|farmRequire\.i|farmRequire\.f" --type mdLength of output: 206
FYI: Documentation for new
farmRequirehelper methods not foundOur search for documentation of the new helper methods (
farmRequire._m(),farmRequire.i(), andfarmRequire.f()) in markdown files returned no results. Please manually verify that the Farm v2.0 documentation (or another appropriate reference) includes details about these methods. If they are indeed missing, update the docs to reflect this breaking change so that developers have proper guidance on the new module interoperability features.
- Confirm that Farm v2.0 documentation covers these new helper methods.
- Update or add documentation as needed to avoid confusion.
crates/compiler/tests/fixtures/external/node/normala/esm/output.js (6)
2-2: Node.js-specific module initializationThe code now correctly uses the modern Node.js
createRequireAPI from the 'module' package to create a require function based on the current module's URL. This approach is appropriate for ESM in Node.js.
3-6: Consistent module system initializationThe module system initialization follows the same pattern as the browser version, maintaining consistency across environments while still being appropriate for Node.js.
7-7: ESM-style external module importThe code now properly uses ESM import syntax to import jQuery, which is appropriate for an ESM output file in Node.js.
8-10: Node.js global namespace usageThis section correctly uses the Node.js
globalobject instead ofwindowfor registering external modules, properly adapting the approach for Node.js environment.
11-21: Consistent module definition structureThe module definition structure is consistent with the browser version, which is good for maintainability across different environments.
23-23: Environment-specific bootstrappingThe bootstrapping process correctly uses the Node.js
globalobject instead ofwindowto access the module system, while maintaining the same overall approach as the browser version.crates/compiler/tests/fixtures/_internal/runtime/src/modules/plugin.ts (1)
1-3: Module system initialization looks goodThis function is part of the module system initialization that logs which file is being processed. It's simple but effective for debugging purposes.
crates/compiler/tests/bundle.rs (6)
17-17: Function signature enhancement with optional config modifierThe test function's signature has been updated to accept an optional closure parameter for modifying the config, which enhances its flexibility.
36-37: Mode change from Production to DevelopmentThe test mode has been changed from Production to Development, which will affect optimization, sourcemaps, and other build features.
49-50: Target environment and resolve configuration updatesTwo important changes:
- Target environment changed from "library-browser" to "library-node"
- Added auto_external_failed_resolve = true for more graceful handling of unresolvable imports
These changes align with the v2.0 development objectives.
61-63: Config modification callback implementationThe new callback pattern allows for flexible configuration modifications in tests, improving test maintainability.
83-85: New single_bundle_test helper functionThis helper function simplifies testing with a single bundle by providing None for the config modifier.
87-98: New multiple_bundle_test helper functionThis function implements a specific test configuration for multi-bundle scenarios, setting up named resources with regex tests.
crates/compiler/src/generate/partial_bundling.rs (12)
7-11: New imports forModuleGraph,ModuleGroup,ModuleId, andHashSet
These additions appear necessary and introduce no obvious issues or conflicts.Also applies to: 17-17
62-62: Introducing dynamic entry resource pots
The new invocationgenerate_dynamic_entry_resource_pots(context)properly isolates dynamic entries. This separation can help keep the bundling logic clear.
68-70: Order of filling fields for dynamic entries
The decision to fill necessary fields for regular resource pots before adding dynamic entry pots appears logical. Just ensure that any required fields for dynamic entries are indeed filled insidegenerate_dynamic_entry_resource_pots.
188-188: Using a HashSet formodules
AHashSetis a solid choice to avoid duplicates and maintain quick lookups here.
195-199: SkippingDynamicEntrymodule groups ingenerate_enforce_resource_pots
This make sense to ensure dynamic entries are kept separate, preventing accidental bundling within enforced resource pots.
236-254: New functiongenerate_dynamic_entry_resource_pots
This function cleanly generatesResourcePotobjects for allDynamicEntrygroups. The approach is straightforward and appears maintainable.
297-297: Test imports forfarmfe_testing_helpers
Bringing inassert_resource_potsandconstruct_test_module_graph_complexis appropriate for thorough testing.
305-305: Using.into_keys()to collect all entry keys
This is a neat way to handle entries for partial bundling testing.
336-336: Assertion for expected resource pot length
Ensures the bundling logic produces the correct number of pots. No issues.
346-346: Repeated.into_keys().collect()usage
Matches the approach in prior tests, remains consistent.
374-374:assert_resource_pots!macro usage
Helps validate pot structure. No concerns.
121-145:✅ Verification successful
Skipping dynamic entries when filling resource pot fields
By design, these lines ignore dynamic entry module groups to avoid double-initialization. Confirm that modules that belong to both an entry and dynamic entry group cannot slip through undetected.Generate and run a script to search for references to dual membership, if any:
🏁 Script executed:
#!/bin/bash # Searching for modules that appear in both entries and dynamic entries ast-grep --pattern $'module_graph.entries[$_] = $_;' rg -A 5 "dynamic_entries"Length of output: 19954
Dual Membership Check Passed
The search results confirm that no module is inadvertently included as both an entry and a dynamic entry. The logic incrates/compiler/src/generate/partial_bundling.rsproperly ignores dynamic entry module groups when filling resource pot fields, ensuring that a module belonging to both types cannot slip through undetected.crates/compiler/tests/fixtures/css/prefixer/normal/output.js (5)
1-2: IntroducingmoduleSystemobject
This global object sets the stage for later calls. Looks fine.
3-6:initModuleSystemand immediate invocation
Initializes the module system by logging a message. The approach is straightforward, ensuring consistency with other module-system initialization patterns.
9-9: Comment for the CSS file reference
Renaming or clarifying the file reference with//index-337c.css:remains purely informational.
33-34: Closing bracket for the CSS keyframes
No functional changes, purely structural.
35-47: Immediate function invocation for bundling
This pattern consistently registers modules withmoduleSystem.g. The change merges the import statements and bundling calls. No apparent problems.crates/compiler/tests/fixtures/library/browser/external/output.js (4)
2-11: Module system initialization looks goodThe module system initialization appears correct. Two initialization functions are defined and called with the moduleSystem object.
11-13: External module setup is properly handledThe code correctly sets up the 'node:fs' module in the window namespace, providing a fallback empty object if it doesn't exist.
14-25: Module registration pattern is clean and efficientThe module registration logic iterates through the modules object and registers each module with the moduleSystem using the
gmethod. The implementation of the 'index.ts' module correctly requires the 'node:fs' module and logs its output.
26-26: Module system bootstrapping is conciseThe code efficiently initializes the module system and sets the entry point to "index.ts".
crates/compiler/tests/fixtures/library/decl/normal/output.js (2)
1-22: Variable declarations and function definitions look correctThe code correctly demonstrates variable scoping with multiple declarations using suffixed identifiers (a, a$1, a$2) and function scopes that shadow outer variables.
23-52: Potential infinite loop in for loop implementationWhile most of the for loop implementations look correct, there's a potential issue with the last for loop.
The for loop at lines 50-52 uses a constant variable (
const for3$2) without an increment expression:for(const for3$2 = 123; for3$2 < 234; for3$2){ break; }While the
breakstatement prevents an infinite loop here, using a constant in a for loop that would normally be incremented is unusual. Is this intentional for testing purposes? If this is meant to be a real loop that should iterate, consider changing to:for(let for3$2 = 123; for3$2 < 234; for3$2++){ break; }crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (3)
1-7: Module system initialization is consistent with other filesThe module system initialization follows the same pattern as in other files, maintaining consistency across the codebase.
9-12: CSS URL handling looks correctThe CSS includes an absolute URL path ("/font.ttf") for the background image, which is appropriate for the URL replacer test case.
14-26: Module registration and bootstrapping follows the established patternThe code correctly imports the runtime and CSS files, registers the module with ID "b5d64806", and initializes the module system.
crates/compiler/src/update/patch_module_group_graph/test_remove_and_add.rs (3)
18-105: Well-structured test graph construction.The
construct_remove_then_add_test_module_graphfunction creates a comprehensive test graph with nodes I, J, and K, including various types of dependencies (standard imports, dynamic imports) and even a cyclic dependency between K and J. This provides excellent test coverage for complex graph scenarios.
107-168: Thorough test setup and backup graph handling.The test properly creates a backup of the graph before modifications, ensuring that the original state can be compared with the modified state later. The removal of multiple modules and edges followed by execution order updates demonstrates a comprehensive testing approach.
169-223: Complete verification of graph patching operations.The test validates the affected groups after patch operations and correctly verifies the updates when adding back dynamic modules. The assertion of affected groups includes the correct set of module group IDs, including all those that should be impacted by the changes.
crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (2)
1-7: Simplified module system initialization.The new implementation introduces a cleaner approach with a dedicated
initModuleSystemfunction and properly initializes the module system. This is more maintainable than the previous implementation with multiple interop functions.
18-37: Streamlined module registration mechanism.The new IIFE (Immediately Invoked Function Expression) approach for module registration improves code organization by directly interacting with the
window['__farm_default_namespace__']object. This is more efficient and easier to maintain than the previous approach.crates/compiler/src/generate/render_resource_pots.rs (2)
89-121: Robust handling of output filename transformation.The implementation properly handles different output filename configurations, including entry-specific filenames and fallbacks. The code also properly considers both static entries and dynamic entries when resolving entry names.
183-217: Simplified function signature and resource rendering.Removal of
skip_renderandchunk_resource_infoparameters simplifies the function signature and improves usability. The rendering process directly utilizes the resource pot and its properties, eliminating unnecessary intermediate structures.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (4)
51-67: Comprehensive test coverage for dynamic entries.The addition of module "I" and a dynamic entry edge from "E" to "I" enhances the test coverage by including dynamic entry scenarios. This ensures the functionality of handling dynamic entries is properly tested.
144-159: Well-structured test data organization.The introduction of structured types (
GroupResourcePotsSnapshotItemandModuleResourcePotSnapshotItem) with proper ordering implementations significantly improves test data organization and readability.Also applies to: 190-204
162-188: Consistent approach to collecting and asserting module group data.The approach to collecting and asserting module group data uses a consistent pattern, building up a collection of
GroupResourcePotsSnapshotIteminstances for each module group, making the test more maintainable.
346-434: Good test coverage for module removal scenarios.The new test function
test_generate_and_diff_resource_pots_remove_moduleprovides excellent coverage for the module removal case, ensuring that the affected groups are correctly identified and that removed modules are no longer present in the module graph.crates/compiler/tests/fixtures/css/modules/normal/output.js (4)
1-7: Streamlined module system initializationThe new implementation introduces a more direct approach to module system initialization with a cleaner separation of concerns. This simplifies the code by removing the previous complex interop functions (
_interop_require_default,_export_star, etc.) that were used for module handling.
21-26: Improved module registration patternThe new IIFE pattern for module registration provides a cleaner encapsulation of the module registration process. This approach centralizes the module registration logic and improves code organization compared to the previous implementation.
27-41: Simplified module export mechanismThe new module system uses a more direct approach for module exports with
farmRequire._m(exports)to set up modules and provides a cleaner way to handle CSS module imports. The implementation of modules for CSS classes is now more maintainable.
42-42: Improved module execution flowThe module execution flow is now more explicit with clear separation between system initialization, module registration, and execution. This makes the code easier to understand and maintain.
crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (3)
1-7: Streamlined module system initializationThe implementation follows the same pattern as other files, providing a consistent approach to module system initialization across the codebase. This consistency improves maintainability.
15-20: Clean module registration patternThe module registration pattern uses a consistent approach across files, making the code more predictable and easier to maintain.
21-25: Simplified module implementationThe module implementation is straightforward and focuses on what's needed without unnecessary complexity. The removal of previous interop functions makes the code more maintainable.
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (3)
1-7: Consistent module system initializationThe module system initialization follows the same pattern as other files, maintaining consistency throughout the codebase. This approach replaces the previous complex interop functions with a simpler design.
18-23: Improved module registration patternThe module registration pattern follows a consistent approach, enhancing code predictability and maintainability.
24-44: Improved handling of CSS module dependenciesThe new implementation handles CSS module dependencies in a cleaner way, with explicit imports between modules using
farmRequire.i()and access to exported values usingfarmRequire.f(). This makes the dependency relationships more explicit and easier to follow.crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (4)
1-7: Consistent module system initializationThe module system initialization is consistent with other files, providing a unified approach across the codebase. This consistency improves maintainability and reduces cognitive load when working with different parts of the system.
18-23: Clean module registration approachThe module registration approach is consistent with other files, using an IIFE to encapsulate the registration logic. This pattern improves code organization and maintainability.
24-36: Simplified CSS class compositionThe handling of CSS class composition is now more straightforward, with explicit module relationships. The implementation provides a cleaner way to manage CSS class dependencies between modules.
37-37: Improved module execution flowThe module execution flow follows a consistent pattern across files, making the code more predictable and easier to maintain.
crates/compiler/src/update/patch_module_group_graph/test_dynamic_entries.rs (4)
1-17: Comprehensive imports for testing dynamic entries.The imports are well-organized and include all necessary components for testing module graph patching with dynamic entries. The inclusion of
farmfe_plugin_partial_bundling::module_group_graph_from_module_graphis particularly important for these tests.
18-159: Well-structured test for complex dynamic entry patching.This test thoroughly verifies the behavior of
patch_module_group_graphwith dynamic entries in a complex scenario. It covers:
- Building an initial graph with a dynamic entry
- Creating an updated graph with different modules and entries
- Verifying the correct patching of the module graph
- Checking that dynamic entries are properly updated
- Validating that module groups are appropriately affected
- Ensuring each module belongs to the correct groups
The test demonstrates a good pattern for setting up module graph tests with explicit entry points and verifiable assertions.
161-272: Good test coverage for dynamic entry updates.This test specifically focuses on updating existing dynamic entries, ensuring that the module graph correctly reflects changes when an edge is transformed into a dynamic entry. The assertions verify that:
- The module graph entries remain unchanged
- The dynamic entries are correctly updated
- The affected groups include all relevant groups
- The module-to-group mappings are accurate
This test complements the first one by focusing on the entry update use case rather than addition/removal.
274-375: Complete test case for dynamic entry removal.This test effectively covers the case of removing dynamic entries from the module graph. It verifies:
- The correct emptying of the dynamic entries map
- Appropriate updates to affected module groups
- Correct module-to-group mappings after the removal
The consistency in testing approach across all three tests makes the functionality coverage comprehensive and easy to maintain.
crates/compiler/tests/fixtures/css/modules/global/output.js (4)
1-7: Simplified module system initialization.The new implementation initializes a module system with a minimal structure. This replaces the more complex interop module system that was previously in place, providing a cleaner entry point for the runtime.
9-18: CSS modules properly preserved.The CSS module content is correctly maintained in the output file, ensuring that styles are available to the application.
20-37: Streamlined module registration mechanism.The new module system implementation uses a more direct approach for registering modules with the Farm runtime. This change aligns with the module system simplification seen throughout the PR:
- It imports the runtime directly
- Uses an IIFE to register modules with the module system
- Maintains the same module content structure
This is a significant improvement over the previous implementation which used multiple helper functions.
38-38: Efficient module system initialization and entry point setup.The final line properly initializes the module system and sets up the entry point in a single statement, eliminating the need for separate initialization steps.
crates/compiler/src/update/regenerate_resources/mod.rs (5)
3-18: Updated imports for enhanced source map handling.The imports have been reorganized to include enhanced magic string functionality and source map generation utilities. These imports support the improved source map generation in the modified code.
21-24: Properly updated import to match the refactored render_resource_pot function.The import statement has been updated to include the new function signatures from the render_resource_pots module, ensuring compatibility with the refactored code.
32-33: Clear documentation update for target environment limitations.The documentation clearly states that the function works only with the browser target environment, helping users understand the function's limitations.
188-202: Improved handling of module resource pots.The code now correctly iterates over all resource pots for each module, rather than assuming a single resource pot. This change aligns with the updated data model where modules can belong to multiple resource pots.
234-247: Default initialization for module resource pots.The field initialization has been updated to use
Default::default()for consistency with the rest of the codebase. This change also reflects the shift to using a collection of resource pots rather than a single one.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (9)
1-26: Well-organized imports for resource pot handling.The imports have been reorganized to better reflect the module's dependencies, with a clear separation between core farmfe imports and local module imports. The addition of
dynamic_entry_module_group_to_resource_potis particularly important for the new functionality.
51-53: Added support for dynamic entry resource pots.This change introduces handling for dynamic entry resource pots, which is a critical enhancement for the module graph patching system. The addition of
handle_dynamic_entry_resource_potsextends the framework to properly manage dynamic entries.
74-80: Always render dynamic entry resource pots.The code now ensures dynamic entry resource pots are always rendered, which is essential for maintaining a consistent module system state. This change complements the previous addition of dynamic entry resource pot handling.
90-108: Skip dynamic entry module groups in affected modules collection.The function now properly ignores dynamic entry module groups when collecting affected modules, which is a significant improvement in the resource pot handling logic. This change makes the system more efficient by focusing only on relevant module groups.
117-129: Improved module external check for better compatibility.The
is_module_externalfunction has been refactored to support checking removed modules, making it more versatile and reliable. This change ensures consistent behavior for both existing and removed modules.
229-229: Proper integration of empty resource pot removal.The call to
remove_empty_resource_potsappropriately integrates the extracted function into the resource pot handling flow, ensuring that empty pots don't linger in the system.
240-303: Complete implementation of dynamic entry resource pot handling.The new
handle_dynamic_entry_resource_potsfunction properly manages the lifecycle of dynamic entry resource pots, ensuring that:
- Dynamic entries are correctly identified from affected module groups
- Appropriate resource pots are created and populated
- Old resource pots are properly removed
- Module and module group references are updated
This is a critical addition for supporting dynamic code-splitting patterns.
375-402: Comprehensive resource pot removal implementation.The
remove_resource_potfunction thoroughly handles resource pot removal, including:
- Removing the resource pot from the map
- Cleaning up associated resources
- Updating module references to the removed pot
This ensures no orphaned references remain in the system, preventing memory leaks and inconsistent state.
405-409: Added test modules for comprehensive test coverage.The addition of test modules for
handle_dynamic_entry_resource_potsandhandle_enforce_resource_potsdemonstrates a commitment to thorough testing of the new functionality, which is essential for ensuring the reliability of these complex features.crates/compiler/tests/fixtures/css/modules/local/output.js (4)
1-2: Looks Good – Introductory LinesThese initial lines introduce the runtime index reference and create a
moduleSystemobject. There are no apparent issues here.
9-9: Comment Only – No ConcernsThis comment indicates a CSS file reference and has no functional impact. Looks good to keep for clarity.
21-21: Minor CSS block closureThis closing brace finalizes the
.farm-helloCSS rule. Nothing problematic here.
23-46: Validate module system usage and readability
- The self-invoking function passes
window["__farm_default_namespace__"].masmoduleSystem, callingmoduleSystem.g(...), which indicatesg(...)must exist on that object. Verify thatmoduleSystem.gis defined in upstream code to avoid runtime errors.- Consider adding whitespace or using multiple lines for clarity around the combined import statements:
import "./farm_internal_runtime_index.js"; import "./index-2528.css"; (function(moduleSystem, modules) { ... })(...);- Confirm that
farmRequire._m,farmRequire.i, andfarmRequire.fare declared and handle errors for missing modules or incorrect references, as these references appear to be required extension points.Would you like a shell script to scan for
moduleSystem.gdefinitions and verify that these dependencies exist?crates/compiler/src/write/mod.rs (8)
1-11: Imports and Basic SetupAll imported crates look correct for writing resources. No immediate issues with
HashMapusage, concurrency, or I/O references.
17-19: Threshold Handling
SMALL_FILE_THRESHOLDis set to a fixed size of 8192. The TODO acknowledges potential differences on various platforms. No immediate concerns, but keep the note in mind for cross-platform testing.
23-32:write()Method Implementation
- The method calls
write_resources_to_disk()and then runsplugin_driver.finish(...)– all part of the final compilation step.- The
Ok(())indicates robust usage ofResult, which is good for capturing errors.
60-84:copy_public_dir()Method
- Verifies public directory existence and separation before copying.
- Prints a warning if
outDiroverlaps withpublicDir.- Overall logic is sound, but you could switch to a logging facade once the “Farm rust logger” is integrated.
86-100:are_separate_folders()MethodMakes a relative path check to confirm two folders don’t overlap. The approach is robust, returning
falseif one is a subfolder of the other.
125-144:create_output_directories()Method
- Iterates over all resource keys and creates directories as needed.
- The
.unwrap()call oncreate_dir_allcan be replaced with a?operator to propagate errors.HashSetusage for directory deduplication is efficient.
146-167:split_resources()Method
- Cleanly filters out already
emittedresources.- Separates small vs. large files based on
SMALL_FILE_THRESHOLD.- Straightforward logic, minimal changes recommended.
182-192:write_small_files()MethodThe sequential approach is consistent with the prior logic. Similar to large files, consider capturing I/O errors instead of ignoring them.
crates/compiler/src/update/patch_module_group_graph.rs (22)
1-2: Imports & Deque UsageThe added imports for
VecDequeand relevant data structures facilitate BFS/DFS logic for patching the module graph. No issues here.
3-4: Module GraphEdge & ModuleGroupTypeThese expansions align with dynamic import handling for patching. Looks correct.
11-12: HashMap & HashSet UsageAdopting the internal
HashSet::default()as an initializer is conventional for Rust. No concerns.
21-22: Return Type Updated toHashSet<ModuleGroupId>Shifting from returning
HashSet<ModuleId>toHashSet<ModuleGroupId>clarifies that the function identifies affected module groups rather than modules. This seems consistent with the new logic.
32-43: Loop Through Dependency Changes
- The loop processes removed edges first, then added edges. This ordering helps avoid new edges being immediately undone by removal logic.
- Calls
patch_removed_dynamic_import_and_dynamic_entryandpatch_added_dynamic_import_and_dynamic_entry. The separation of concerns is good.
46-57: Add & Remove Edge Logic in DependenciesEffectively merges the changes from
diff_result.deps_changes. Straightforward approach.
60-64: Filtering Out Non-Existent ModuleGroups
module_group_graph.has(g_id)ensures you only return module groups still present in the graph. This is crucial to avoid referencing stale IDs.
66-67: Initialization of the Final QueueCollecting the “affected” groups in a
VecDequefosters BFS expansion. Straightforward BFS logic.
68-83: Propagating Affected Module GroupsEach dequeued group triggers inspection of its modules. This BFS approach ensures that related or parent groups get added to the queue. The logic is thorough.
88-100:get_previous_module_groups()Looks up the existing or removed module’s groups to preserve context during patching. Works properly to unify the logic for both present and removed modules.
102-158:add_dynamic_module_group()Method
- Creates new module groups for dynamic imports or dynamic entries.
- If the group already exists, edges are updated as needed (rather than overwriting). This is correct for maintaining graph consistency.
- Good that you differentiate
diff_result.added_modules.contains(added_module_id)from already-existing modules.
160-183:remove_dynamic_module_group()MethodRemoves a module group, cleans associated modules, and updates the
affected_module_groups. Ensures no leftover references remain.
185-259:patch_added_dynamic_import_and_dynamic_entry()Method
- The separation of dynamic-entry creation (
ModuleGroupType::DynamicEntry) and dynamic-import creation (ModuleGroupType::DynamicImport) is very clear.- Patching the parent groups for static imports ensures the newly added module is included in existing groups. This is thorough.
- The BFS-like approach for non-dynamic children is well-structured.
261-372:patch_removed_dynamic_import_and_dynamic_entry()Method
- Properly checks if modules remain in use by other dynamic parents before removing their module group.
- If the entire module is removed, an inline cleanup removes the module from each group.
- The approach is consistent and logically complements the “added” patch logic.
375-458:patch_existing_added_non_dynamic_children()Method
- Queues child modules, ensuring each is patched into parent groups if it’s missing.
- Distinguishes between dynamic and static children to handle them differently.
- The BFS approach with a visited set helps avoid infinite loops in cyclical graphs.
460-480:patch_existing_removed_non_dynamic_children()MethodRelies on a topological sort to remove any child modules that no longer have valid static parents. Good approach for preventing orphan references.
482-498:get_dynamic_entry_group_ids()MethodExtracts only those group IDs that have type
DynamicEntry– concise helper function.
500-524:patch_dynamic_entry_group_for_added_dynamic_import()MethodBridges dynamic children to dynamic entry groups, ensuring newly discovered modules are appended via
patch_existing_added_non_dynamic_children. Maintains consistent BFS.
526-548:patch_dynamic_entry_group_for_removed_dynamic_import()MethodPerforms the topological sort for the removed module, ensuring dynamic entry groups are updated or removed if empty.
550-569:topo_sort_removed_module()HelperUses a DFS topological sort to track removal order. The filtering function helps skip dynamic imports or handle them conditionally.
571-646:patch_any_module_group_for_toposort_modules()MethodCarefully removes modules from relevant module groups if the static dependencies no longer remain. Also checks for leftover dynamic edges. Thorough cleanup.
648-671:should_ignore_cyclic_dependencies()MethodChecks if both
module_idanddep_idappear in the same cycle, ignoring certain edges in cyclical pairs. The logic ensures you don’t prematurely remove a module in cyclical relationships.
| default: dep_ts_default, | ||
| __esModule: true | ||
| }; | ||
| var importNamespace_ts_default = dep_ts_namespace_farm_internal_; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify the necessity of unused variable
The variable importNamespace_ts_default is assigned but never used within this file. Consider whether this assignment is necessary for the test case.
🏁 Script executed:
#!/bin/bash
# Check if this variable is referenced in related test files
grep -r "importNamespace_ts_default" crates/compiler/tests/fixtures/library/export/Length of output: 218
Action Item: Remove or Clarify the Unused Variable
After verifying its usage, the variable importNamespace_ts_default is assigned in crates/compiler/tests/fixtures/library/export/entryExport3/output.js (line 21) but isn’t referenced elsewhere in the fixture. Please assess whether this assignment was intended for a side effect or if it can be removed altogether. If it’s required for the test scenario, consider adding an inline comment explaining its purpose.
- Confirm the intent behind
importNamespace_ts_defaultin this test case. - Remove the assignment if it serves no functional purpose.
| (function(){const moduleSystem = {}; | ||
| function initModuleSystem() { | ||
| console.log('module-helper.ts'); | ||
| } | ||
| initModuleSystem(moduleSystem); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Module system initialization could be improved
The IIFE creates a moduleSystem object but doesn't pass it to the initModuleSystem function. This is inconsistent with the function call on line 6 which suggests the function expects a parameter.
(function(){const moduleSystem = {};
function initModuleSystem() {
console.log('module-helper.ts');
}
-initModuleSystem(moduleSystem);
+initModuleSystem();Alternatively, if the function is meant to receive the module system:
(function(){const moduleSystem = {};
-function initModuleSystem() {
+function initModuleSystem(ms) {
console.log('module-helper.ts');
}
initModuleSystem(moduleSystem);📝 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(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){ | |
| const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(); | |
| })(); |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){ | |
| const moduleSystem = {}; | |
| function initModuleSystem(ms) { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| })(); |
| (function(){const moduleSystem = {}; | ||
| function initModuleSystem() { | ||
| console.log('module-helper.ts'); | ||
| } | ||
| initModuleSystem(moduleSystem); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Module system initialization inconsistency
The IIFE creates a moduleSystem object but doesn't pass it to the initModuleSystem function. This is inconsistent with the function call on line 6 which suggests the function expects a parameter.
(function(){const moduleSystem = {};
function initModuleSystem() {
console.log('module-helper.ts');
}
-initModuleSystem(moduleSystem);
+initModuleSystem();Alternatively, if the function is meant to receive the module system:
(function(){const moduleSystem = {};
-function initModuleSystem() {
+function initModuleSystem(ms) {
console.log('module-helper.ts');
}
initModuleSystem(moduleSystem);📝 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(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){const moduleSystem = {}; | |
| -function initModuleSystem() { | |
| +function initModuleSystem(ms) { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); |
| (function(){const moduleSystem = {}; | ||
| function initModuleSystem() { | ||
| console.log('module-helper.ts'); | ||
| } | ||
| initModuleSystem(moduleSystem); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Module system initialization inconsistency
The IIFE creates a moduleSystem object but doesn't pass it to the initModuleSystem function. This is inconsistent with the function call on line 6 which suggests the function expects a parameter.
(function(){const moduleSystem = {};
function initModuleSystem() {
console.log('module-helper.ts');
}
-initModuleSystem(moduleSystem);
+initModuleSystem();Alternatively, if the function is meant to receive the module system:
(function(){const moduleSystem = {};
-function initModuleSystem() {
+function initModuleSystem(ms) {
console.log('module-helper.ts');
}
initModuleSystem(moduleSystem);📝 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(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(); |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem(ms) { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); |
| (function(){const moduleSystem = {}; | ||
| function initModuleSystem() { | ||
| console.log('module-helper.ts'); | ||
| } | ||
| initModuleSystem(moduleSystem); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Module system initialization inconsistency
The IIFE creates a moduleSystem object but doesn't pass it to the initModuleSystem function. This is inconsistent with the function call on line 6 which suggests the function expects a parameter.
(function(){const moduleSystem = {};
function initModuleSystem() {
console.log('module-helper.ts');
}
-initModuleSystem(moduleSystem);
+initModuleSystem();Alternatively, if the function is meant to receive the module system:
(function(){const moduleSystem = {};
-function initModuleSystem() {
+function initModuleSystem(ms) {
console.log('module-helper.ts');
}
initModuleSystem(moduleSystem);📝 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(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(); |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem() { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); | |
| (function(){const moduleSystem = {}; | |
| function initModuleSystem(ms) { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); |
| //index.js: | ||
| (function(){const moduleSystem = {}; | ||
| function initModuleSystem() { | ||
| console.log('module-system-helper.ts'); | ||
| } | ||
| function initModuleSystem$1() { | ||
| console.log('module-helper.ts'); | ||
| } | ||
| initModuleSystem(moduleSystem); | ||
| initModuleSystem$1(moduleSystem); | ||
| }());(function(moduleSystem, modules) { | ||
| for(var moduleId in modules){ | ||
| var module = modules[moduleId]; | ||
| moduleSystem.g(moduleId, module); | ||
| } | ||
| })(window["__farm_default_namespace__"].m, { | ||
| "index.ts": function(module, exports, farmRequire, farmDynamicRequire) { | ||
| console.log('hello world'); | ||
| } | ||
| }); | ||
| var __farm_ms__ = window['__farm_default_namespace__'].m;__farm_ms__.b();var __farm_entry__=__farm_ms__.r("index.ts"); |
There was a problem hiding this comment.
Function parameter mismatch in module initialization
The initModuleSystem and initModuleSystem$1 functions are called with the moduleSystem argument (lines 9-10), but their declarations don't accept any parameters (lines 3-8). This mismatch could lead to unexpected behavior.
This output file also makes assumptions about global objects and methods:
- Assumes
window["__farm_default_namespace__"].mexists with a methodg - Assumes
__farm_ms__has methodsbandr
These should be verified to exist in the runtime environment.
| } | ||
| ,});window['__farm_default_namespace__'].__farm_module_system__.setInitialLoadedResources([]);window['__farm_default_namespace__'].__farm_module_system__.setDynamicModuleResourcesMap([],{ });var farmModuleSystem = window['__farm_default_namespace__'].__farm_module_system__;farmModuleSystem.bootstrap();var entry = farmModuleSystem.require("b5d64806"); | ||
| initModuleSystem(moduleSystem); |
There was a problem hiding this comment.
Mismatch between function signature and call parameters
initModuleSystem(moduleSystem); suggests a parameter is expected, but initModuleSystem is declared without arguments. This likely indicates a missing parameter definition in the function or an unintended unused argument.
-function initModuleSystem() {
+function initModuleSystem(system) {
console.log('module-helper.ts');
}📝 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.
| initModuleSystem(moduleSystem); | |
| function initModuleSystem(system) { | |
| console.log('module-helper.ts'); | |
| } | |
| initModuleSystem(moduleSystem); |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/compiler/src/update/regenerate_resources/mod.rs (1)
78-139:⚠️ Potential issueHandle potential errors more gracefully in gen_resource_pot_code
The rewritten closure has several
.unwrap()calls that could panic if the operations fail. Consider handling these errors more gracefully by using?operator or explicit error handling.Apply this diff to improve error handling:
- let sourcemap = update_resources - .source_map - .map(|map| SourceMap::from_slice(&map.bytes).unwrap()); + let sourcemap = update_resources + .source_map + .map(|map| SourceMap::from_slice(&map.bytes) + .map_err(|e| CompilationError::GenericError(format!("Failed to parse source map: {}", e)))?); - 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)))?; - let map = magic_string - .generate_map(SourceMapOptions::default()) - .unwrap(); + let map = magic_string + .generate_map(SourceMapOptions::default()) + .map_err(|e| CompilationError::GenericError(format!("Failed to generate map: {}", e)))?; - sourcemap.to_writer(&mut buf).unwrap(); - let map_code = String::from_utf8(buf).unwrap(); + sourcemap.to_writer(&mut buf) + .map_err(|e| CompilationError::GenericError(format!("Failed to write sourcemap: {}", e)))?; + let map_code = String::from_utf8(buf) + .map_err(|e| CompilationError::GenericError(format!("UTF-8 decoding issue for sourcemap: {}", e)))?;
🧹 Nitpick comments (5)
crates/compiler/src/update/mod.rs (2)
159-159: Remove commented code or clarify intention.There's a commented-out line
self.context.set_update();. If this function is no longer needed, the comment should be removed. If it's meant to be used later, add a TODO comment explaining why it's commented out.- // self.context.set_update();
673-682: Improve error handling in asynchronous resource regeneration.Current implementation just prints errors to console but doesn't propagate them properly, which could lead to silent failures and difficult debugging. Consider storing these errors in a thread-safe collection that can be accessed after thread completion.
if let Err(e) = regenerate_resources_for_affected_module_groups( affected_module_groups, diff_result, &cloned_updated_module_ids, &removed_modules, &cloned_context, ) { println!("Failed to regenerate resources: {e}"); println!("modules to regenerate: {cloned_updated_module_ids:?}"); + // Store error in a thread-safe collection or notify main thread + cloned_context.error_channel.send(e).unwrap_or_else(|send_err| { + println!("Failed to send error: {send_err}"); + }); }crates/compiler/src/update/patch_module_group_graph.rs (3)
129-185: Dynamic module group addition logic could be optimized.While the logic is correct, there's some redundancy in checking module existence multiple times.
Consider caching module lookups to avoid repeated
module_graph.module()calls for the same module ID, especially in closures likeadd_edges.let add_edges = |module_group_graph: &mut ModuleGroupGraph| { // only add edge for dynamic import if !matches!(module_group_type, ModuleGroupType::DynamicEntry) { - let module_group_ids = { - let module = module_graph - .module(module_id) - .unwrap_or_else(|| panic!("module {module_id:?} not found")); - module.module_groups.clone() - }; + // Get module once to avoid repeated lookups + let module = module_graph + .module(module_id) + .unwrap_or_else(|| panic!("module {module_id:?} not found")); + let module_group_ids = module.module_groups.clone(); for module_group_id in &module_group_ids {
333-339: Consider simplifying repeated code pattern.The pattern of retrieving module group IDs from either removed modules or current modules appears multiple times in the codebase.
This logic could be extracted to a helper function similar to
get_previous_module_groupsto reduce duplication and improve maintainability:- let module_group_ids = { - if removed_modules.contains_key(module_id) { - let removed_module = removed_modules.get(module_id).unwrap(); - removed_module.module_groups.clone() - } else { - let module = module_graph - .module(module_id) - .unwrap_or_else(|| panic!("module {module_id:?} not found")); - module.module_groups.clone() - } - }; + let module_group_ids = get_previous_module_groups(module_id, module_graph, removed_modules);
402-485: Good handling of visited nodes to prevent recursion issues.The use of a visited set in the breadth-first traversal is vital to prevent potential infinite loops in circular dependency structures. Consider adding a brief comment explaining why the visited set is necessary.
Add a comment explaining the purpose of the visited set:
fn patch_existing_added_non_dynamic_children( added_module_id: &ModuleId, previous_parent_groups: HashSet<ModuleGroupId>, module_graph: &mut ModuleGraph, module_group_graph: &mut ModuleGroupGraph, affected_module_groups: &mut HashSet<ModuleGroupId>, visited: &mut HashSet<ModuleId>, ) { + // The visited set prevents processing the same module multiple times + // which is important to avoid infinite loops in circular dependencies let mut queue = VecDeque::from([added_module_id.clone()]);
📜 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 (13)
.github/workflows/ci.yaml(4 hunks).github/workflows/rust-build.yaml(9 hunks).github/workflows/rust-test.yaml(1 hunks)crates/compiler/Cargo.toml(2 hunks)crates/compiler/src/generate/partial_bundling.rs(8 hunks)crates/compiler/src/update/diff_and_patch_module_graph.rs(13 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/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(9 hunks)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs(6 hunks)crates/compiler/src/update/regenerate_resources/mod.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/rust-test.yaml
- crates/compiler/src/update/diff_and_patch_module_graph.rs
- .github/workflows/rust-build.yaml
- crates/compiler/src/update/handle_update_modules.rs
🧰 Additional context used
🧬 Code Definitions (3)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (3)
crates/compiler/src/update/patch_module_group_graph.rs (3)
patch_module_group_graph(16-113)module_graph(607-611)module_graph(654-658)crates/compiler/src/update/regenerate_resources/mod.rs (2)
clear_resource_pot_of_modules_in_module_groups(230-245)resource_pot_map(201-205)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (2)
generate_and_diff_resource_pots(34-83)resource_pots(268-271)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (2)
crates/core/src/module/module_group.rs (1)
module_group(79-82)crates/plugin_partial_bundling/src/merge_module_pots.rs (2)
new(24-29)new(70-75)
crates/compiler/src/update/mod.rs (4)
crates/core/src/module/module_graph.rs (1)
module(292-300)crates/core/src/module/module_group.rs (1)
module_group(79-82)crates/compiler/src/update/handle_update_modules.rs (2)
paths(89-92)handle_update_modules(13-182)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (1)
updated_module_ids(199-202)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm-gnueabihf
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Test - windows-latest
- GitHub Check: Test - macos-latest
- GitHub Check: Test - ubuntu-latest
🔇 Additional comments (70)
crates/compiler/src/generate/partial_bundling.rs (8)
7-17: Import changes align with dynamic entry supportThese import changes add the necessary types to support dynamic entry module handling. Using
HashSetfor module groups is a good choice as it avoids duplicates effectively.
62-70: Good separation of dynamic entry handlingThe new approach correctly separates dynamic entry resource pot generation, ensuring proper handling order.
- Dynamic entry pots are generated first
- Regular resource pots have necessary fields filled
- Dynamic entry pots are added afterward since they're already processed
This clear sequence prevents potential side effects during field population.
121-141: Improved filtering of module groupsThis change properly excludes dynamic entry module groups when filling fields for regular resource pots. The filter operation is clear and the comment explains the reasoning well.
145-145: Enhanced error message with improved debugging informationThe panic message now includes the resource pot ID, which significantly improves debugging by providing better context when an error occurs.
195-198: Consistent handling of dynamic entry module groupsThis change ensures dynamic entry module groups are skipped during enforce resource pot generation, maintaining consistency with how they're handled elsewhere in the codebase.
236-254: Well-structured function for dynamic entry resource pot generationThe new function effectively filters and processes only the dynamic entry module groups. The implementation is clear and focused on a single responsibility.
256-283: Clean implementation of dynamic entry module group conversionThe function handles all the necessary steps to create a resource pot from a dynamic entry module group:
- Creates the resource pot with the correct type
- Sets entry module and module groups
- Marks it as a dynamic entry
- Establishes bidirectional relationships between modules and the resource pot
- Updates the module group to reference the resource pot
The early return with
Nonewhen the entry module isn't found in dynamic entries is good defensive programming.
336-336: Minor test updates to match implementation changesThe updates to the test assertions maintain test coverage for the new functionality.
Also applies to: 374-374
crates/compiler/Cargo.toml (2)
17-17: Addition of new script meta plugin.The new
farmfe_plugin_script_metadependency appears to be part of a larger plugin reorganization for Farm v2.0. This is a good modularization approach. Just note that the version is at 0.0.1, which indicates it's in early development.
32-34:❓ Verification inconclusive
New plugins added to replace and extend functionality.
The addition of three new plugins (
farmfe_plugin_mangle_exports,farmfe_plugin_library, andfarmfe_plugin_file_size) at version 0.0.1 suggests a restructuring of functionality. This aligns with the Farm v2.0 changes mentioned in the PR objectives.
🏁 Script executed:
#!/bin/bash # Verify if the removed dependencies' functionality is covered by the new plugins # Check for references to removed dependencies in code echo "Checking for references to removed dependencies..." rg "farmfe_plugin_bundle" --type rust rg "num_cpus" --type rustLength of output: 245
Action Required: Confirm Dependency Usage
The new plugins added to
crates/compiler/Cargo.toml—namely,farmfe_plugin_mangle_exports,farmfe_plugin_library, andfarmfe_plugin_file_size—successfully replace the removed dependency (farmfe_plugin_bundle), as verified by the absence of its references in the codebase. However, the verification script revealed thatnum_cpusis still actively used incrates/core/src/context/mod.rs(vianum_cpus::get()). Please confirm whether this remaining external dependency is intentional or if it should also be addressed as part of the Farm v2.0 restructuring.
- Plugins Added: The new plugins in
Cargo.tomlmatch the PR objectives.- Removed Dependency:
farmfe_plugin_bundlehas been fully removed.- Outstanding Reference:
num_cpusis still present incrates/core/src/context/mod.rs.crates/compiler/src/update/mod.rs (7)
1-23: Update imports and module structure for clarity and functionality.The reorganization of imports and the new module structure (particularly the addition of module group types and finalize_module_graph functionality) improves code organization. The explicit use of HashSet and HashMap from farmfe_core is also cleaner.
78-91: Refactored from record_manager to stats for consistency.Good refactoring to use
statsinstead ofrecord_managerconsistently throughout the codebase for recording compilation metrics and statistics. This makes the code more cohesive and follows a single pattern for stats management.Also applies to: 93-111, 113-143, 155-156
175-182: Improved thread safety with mutex lock.Good implementation of locking the
last_fail_module_idsbefore passing it tohandle_update_modulesand explicitly dropping the lock afterward. This improves thread safety when handling module failures.
237-249: Enhanced error handling with failed module tracking.The addition of
set_last_fail_module_idsin both error and success paths ensures consistent tracking of failure state. This is a good practice for maintaining system state.
268-280: Improved module graph finalization process.The code now uses dedicated functions (
freeze_module_of_affected_module_graphandfinalize_updated_module_graph) for handling module graph updates. This is a good separation of concerns that makes the code more maintainable.
556-611: Enhanced module management with group-level tracking.The method
diff_and_patch_contextnow returnsHashSet<ModuleGroupId>instead ofHashSet<ModuleId>, representing a shift toward higher-level module group management. This abstraction should make the codebase more maintainable and potentially improve performance.
651-663: Updated dynamic resources handling with module group types.The code now uses
ModuleGroupId::new(entry_id, &ModuleGroupType::Entry)instead of directly using entry IDs. This is a more type-safe approach that better represents the relationship between entries and module groups.crates/compiler/src/update/regenerate_resources/mod.rs (5)
3-19: Enhanced imports for improved source map handlingThe addition of imports like
FARM_MODULE_SYSTEM,collapse_sourcemap_chain,MagicString, andSourceMapOptionsindicates a more sophisticated approach to handling source maps and code generation, which will improve the debugging experience.
37-37: Improved error handling with Result return typeChanging the function return type to
farmfe_core::error::Result<(String, String)>is a good practice that allows for proper error propagation up the call stack.
105-113: Improved source map handling with MagicStringThe use of
MagicStringfor code manipulation and source map generation is a significant improvement that provides more flexible and powerful ways to manipulate the code string and generate accurate source maps.
184-198: Support for multiple resource pots per moduleThe changes from a single
resource_potto multipleresource_potsper module represents an architectural improvement that allows modules to be part of multiple resource pots. This provides more flexibility in how resources are organized and bundled.
242-242: Updated resource pot clearingThe update to use
resource_pots = Default::default()aligns with the architectural change to support multiple resource pots per module.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (17)
3-14: Import organization improvedThe imports have been nicely organized by specific modules, making the dependencies clearer. Good job using specific imports rather than wildcard imports.
19-21: Import structure aligns with new functionalitiesThe imports from partial_bundling module correctly reflect the functions used in the implementation, including the new dynamic entry resource pot handling.
51-53: Good addition of dynamic entry resource pot handlingThe dynamic entry resource pot handling has been properly integrated into the main flow, keeping the modular approach consistent with the existing code structure.
80-80: Proper integration of dynamic entry resource pot IDsThe dynamic entry resource pot IDs are correctly added to the set of new resource pot IDs that will be rendered.
93-104: Enhancement to get_affected_modules functionThe function now properly ignores dynamic entry module groups and uses HashSet::default() for initialization. This separation of concerns is a good design choice.
117-129: Improved is_module_external functionThe function has been refactored to take explicit dependencies rather than accessing global state, which is a good practice for testability and maintenance.
131-145: Good extraction of remove_empty_resource_pots logicExtracting this logic into a separate function improves code organization and reusability, following the DRY principle.
158-159: Use of HashSet::default()Using default() for initialization is a good practice as it allows for zero-cost abstractions and is more idiomatic in Rust.
164-164: Updated call to is_module_externalThe function call has been updated to match the new signature, providing better context to the function.
210-210: Consistent usage of is_module_externalThe function call is consistently updated throughout the codebase.
229-229: Improved code reuse with remove_empty_resource_potsUsing the extracted function here improves readability and maintains consistency in the codebase.
240-303: Well-implemented handle_dynamic_entry_resource_pots functionThis function effectively processes dynamic entry module groups to create and update resource pots. The implementation properly:
- Identifies dynamic entry module groups
- Creates resource pots for them
- Updates module group relationships
- Updates module relationships
- Returns affected resource pot IDs
The code is well-structured and handles error cases appropriately.
315-315: Changed from read to write lockThe module graph is now correctly being acquired with a write lock since it is being modified within the function.
319-322: Consistent use of HashSet::default()The code consistently uses default() for initialization, which is good for maintaining style consistency.
344-349: Updated remove_resource_pot callThe function call has been updated to match the new signature, providing necessary context.
375-402: Comprehensive remove_resource_pot implementationThis implementation properly handles resource pot removal, including:
- Removing the resource pot from the map
- Cleaning up related resources
- Updating module references to remove the resource pot
The error handling is good with descriptive panic messages when unexpected conditions occur.
406-409: Test coverage addedGood job adding test modules for the new functionality. This is essential for ensuring code quality and preventing regressions.
crates/compiler/src/update/patch_module_group_graph.rs (6)
22-23: Return type improved for better caller control.The function now returns
HashSet<ModuleGroupId>instead of just modifying state, which allows callers to know which module groups were affected by the patch operation. This is a good improvement for API design.
59-85: Well-structured modularization of dependency handling.The refactoring to separate functions for handling added and removed modules is excellent. The comment about handling removals first shows careful consideration of the execution order to prevent conflicts.
87-113: Comprehensive collection of affected module groups.The BFS traversal ensures that all directly and indirectly affected module groups are properly identified and returned to the caller. This is essential for correct incremental rebuilds.
577-596: Solid topological sorting implementation for dependency handling.The topological sort ensures modules are processed in the correct order, respecting their dependencies. The reversing of the result is essential to process modules in dependency order.
675-698: Excellent handling of circular dependencies.The function to detect and ignore circular dependencies is well-implemented. It prevents cycles from causing issues during module graph patching by determining which dependency direction to ignore.
700-705: Comprehensive test coverage is important.Adding dedicated test modules for dynamic entries and module add/remove operations is essential for maintaining correctness during future changes.
.github/workflows/ci.yaml (4)
6-6: Added new branch target for pull requests.The PR adds
v2-devas a target branch for pull requests alongsidemain. This aligns with the PR title indicating Farm v2.0 development and matches the base branch change in.changeset/config.jsonmentioned in the AI summary.
140-145: New plugin artifact dependency added to ts-test.The same plugin-replace-dirname artifact is also added to the ts-test job, ensuring consistency across test jobs.
208-208: Added plugin to artifact verification matrix.The plugin-replace-dirname has been added to the check-plugin-artifacts matrix, ensuring it will be verified during CI.
58-63:✅ Verification successful
New plugin artifact dependency added to examples-test.
A download step for the plugin-replace-dirname artifact has been added to the examples-test job. This new plugin appears to be part of the v2.0 development effort.
Let's verify that this plugin is properly implemented and required:
🏁 Script executed:
#!/bin/bash # Check if the plugin-replace-dirname directory exists if [ -d "./rust-plugins/replace-dirname" ]; then echo "✅ plugin-replace-dirname directory exists" ls -la ./rust-plugins/replace-dirname else echo "❌ plugin-replace-dirname directory not found" fi # Check if the plugin is referenced in any package.json files echo "Looking for plugin references in package.json files:" rg -l "replace-dirname" --glob "package.json"Length of output: 1605
Verified Plugin Artifact Dependency Implementation
The verification confirms that the
plugin-replace-dirnameis properly implemented and referenced:
- The
./rust-plugins/replace-dirnamedirectory exists and contains the expected files.- Multiple
package.jsonfiles (including those under therust-plugins/replace-dirname/npm/subdirectories) reference “replace-dirname” as expected.- The CI workflow (.github/workflows/ci.yaml, lines 58-63) correctly downloads the artifact and aligns with the v2.0 development efforts.
No further changes are required.
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (7)
10-11: Updated imports to use strongly typed module identifiers.The imports now include
ModuleGroupId,ModuleGroupType, andModuleType, which are used for improved type safety throughout the test.
14-14: Explicitly import HashSet.HashSet is now explicitly imported from farmfe_core instead of being used through a namespace, which improves code readability.
37-43: Enhanced module creation with explicit type assignment.The module creation for "H" has been improved by explicitly setting its type to
ModuleType::Js. This increases code clarity by making the module type obvious.
73-77: Introduced strongly typed module group identifiers.The test now uses
ModuleGroupIdinstances with explicit type information instead of string literals, which improves type safety and makes the code more maintainable.
80-80: Updated assertion to use strongly typed identifiers.The assertion now uses
HashSet::from_iterwithModuleGroupIdinstances instead of string literals, which is more type-safe and consistent with the changes to use strongly typed identifiers.
94-94: Updated collection creation method for consistency.Changed from
HashSet::fromtoHashSet::from_iter, maintaining consistency with other similar operations in the codebase.
204-207: Consistent use of strongly typed identifiers in second test.The second test also now uses a
ModuleGroupIdinstance with explicit type information, maintaining consistency with the earlier changes.crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (14)
10-16: Updated imports for improved module handling.The imports have been updated to include
ModuleGroupId,ModuleGroupType,ModuleId,ResourcePotId, andHashSet, which are used for improved type safety throughout the test.
18-21: Added test helper imports for better assertions.New testing helpers have been imported, including
assert_debug_snapshot,assert_resource_pots, andassert_sorted_iter_eq, which provide more sophisticated assertion capabilities.
28-31: Reorganized imports for clarity.The imports for regenerate_resources have been restructured to be more specific and clearer about what is being imported.
51-68: Added dynamic entry module to test coverage.A new module "I" has been added to the test with a dynamic entry edge from "E" to "I", expanding test coverage to include dynamic entry modules.
91-96: Introduced strongly typed module group identifiers.The test now uses
ModuleGroupIdinstances with explicit type information, including for the new module "I", improving type safety and code maintainability.Also applies to: 99-105
132-142: Improved resource pot handling and assertions.The resource pot handling has been updated to:
- Clear resource pots of modules in affected groups
- Sort resource pot IDs before assertion
- Use snapshot testing for better test failure clarity
These changes make the tests more robust and easier to debug when they fail.
144-159: Added structured group resource pots snapshot item.A new struct
GroupResourcePotsSnapshotItemhas been created with appropriate ordering implementations to help with structured assertions about group resource pots.
167-193: Enhanced group resource pot collection and assertion.The collection and assertion of group resource pots have been significantly improved with a structured approach that:
- Captures group IDs and their resource pots in a structured format
- Uses ordered comparison for reliable assertions
This makes the test more robust and maintainable.
195-209: Added structured module resource pots snapshot item.Similar to the group resource pots, a new struct
ModuleResourcePotSnapshotItemhas been introduced for module resource pots with appropriate ordering implementations.
240-267: Enhanced module resource pot collection and assertion.Module resource pot collection and assertion have been updated with a structured approach, including the newly added module "I", making the test more comprehensive.
298-302: Standardized resource pot sorting and assertion.Resource pots are now sorted consistently before assertion, and a specialized assertion macro
assert_resource_pots!is used for better error messages.
402-404: Consistent use of strongly typed identifiers in second test.The second test now also uses a
ModuleGroupIdinstance with explicit type information, maintaining consistency throughout the codebase.
430-430: Consistent resource pot clearing.The function call
clear_resource_pot_of_modules_in_module_groupsis now used consistently across tests.
443-531: Added test for removing modules from the module graph.A new test case
test_generate_and_diff_resource_pots_remove_modulehas been added to test removing modules D, F, and G from the module graph, significantly improving test coverage of edge cases.
| @@ -125,6 +138,7 @@ fn test_handle_enforce_resource_pots() { | |||
| &context, | |||
| ); | |||
|
|
|||
| // assert_eq!(enforce_resource_pots, vec!["test_js".to_string()]); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Resource pot assertion is commented out.
There's a commented-out assertion that indicates the expected value for enforce_resource_pots might be changing from a specific string to "test_js". This should either be uncommented if the change is implemented or removed if it's no longer relevant.
🏁 Script executed:
#!/bin/bash
# Check if there are TODOs related to this commented assertion
echo "Checking for related TODOs:"
rg "test_js" --type rustLength of output: 275
Action Required: Clarify the Purpose of the Commented-Out Assertion
- The file
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rsstill contains a commented-out assertion on line 141:// assert_eq!(enforce_resource_pots, vec!["test_js".to_string()]); - This comment suggests that the expected value for
enforce_resource_potsmay be intended to change to"test_js". However, there are no additional markers or TODO comments to clarify if this change is final. - Next Steps: Please either:
- Uncomment the assertion if the implementation now intends to validate
"test_js"as the expected value, or - Remove the commented code if it is no longer relevant.
- Uncomment the assertion if the implementation now intends to validate
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
.github/workflows/release.yaml (2)
39-39: Artifact Move for Replace Dirname Plugin: The addedmvcommand for moving artifacts for theplugin-replace-dirnameis a good enhancement. Ensure that the destination directory (./rust-plugins/replace-dirname/npm/${abi}) exists and is consistently used in all relevant parts of the workflow.
64-66: Conditional Configuration in Changesets Action: The conditional expressions for settingversion,publish, andbranchbased ongithub.ref_nameintroduce useful flexibility for distinguishing between nightly and standard releases. It would be beneficial to add an inline comment explaining this logic so that future maintainers can quickly understand the intent behind the ternary-like expressions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/release.yaml(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: Test - windows-latest
- GitHub Check: Test - macos-latest
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Check
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
🔇 Additional comments (2)
.github/workflows/release.yaml (2)
5-6: Branch Trigger Addition: The inclusion of thev2-devbranch alongsidemainin the push trigger is appropriate for Farm v2.0. Please verify that downstream CI/CD pipelines and configurations are updated to correctly handle pushes from both branches.
46-46: Test Check for Replace Dirname Artifact: The added file existence test for./rust-plugins/replace-dirname/npm/${abi}/index.farmis a solid addition to verify the integrity of the moved artifacts.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
crates/compiler/tests/fixtures/css/prefixer/normal/output.js (2)
41-46: Consider using more descriptive method namesThe module system uses non-descriptive method names like
g,_m, which could make the code harder to maintain. Though this is a test fixture, more descriptive names would improve readability.
44-44: Empty string literal seems unnecessaryThe empty string literal on line 44 doesn't appear to serve any purpose in the module function.
- farmRequire._m(exports); - ""; + farmRequire._m(exports);crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (1)
44-45: Enhance code consistency by using the same module creation pattern for module "H"The module creation pattern here differs from line 54. For improved consistency across the codebase, consider using the same pattern you're using for module "H" on lines 37-43 in
test_handle_enforce_resource_pots.rsandtest_handle_dynamic_entry_resource_pots.rs.- let mut m_h = Module::new("H".into()); - m_h.module_type = ModuleType::Js; - update_module_graph.add_module(m_h); + update_module_graph.add_module({ + let mut m = Module::new("H".into()); + m.module_type = ModuleType::Js; + m + });crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_dynamic_entry_resource_pots.rs (1)
139-153: Consider adding test for multiple dynamic entriesThe current test validates a single dynamic entry resource pot. Consider adding a test case that handles multiple dynamic entries to ensure the code works correctly with more complex scenarios.
You could extend the test by adding another dynamic entry relationship and verifying that both are correctly processed.
crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (2)
1-7: Revisit unused parameter and console message.
TheinitModuleSystemfunction simply logs'module-helper.ts'but does not utilize the passed-inmoduleSystem. Consider removing the parameter or using it inside the function. Also verify that referring to a.tsfile in a.jscontext is intentional.
14-26: Ensure global registrations are well-defined.
This block attaches modules towindow["__farm_default_namespace__"]and callsmoduleSystem.g(...), but the snippet does not show the definition of.g. Confirm it exists elsewhere and that the final code correctly registers modules.crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
1-7: Use or remove themoduleSystemparameter.
As before,initModuleSystemlogs'module-helper.ts'but never usesmoduleSystem. Consider either removing the parameter or employing it for module initialization.crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
1-7: AligninitModuleSystemusage.
initModuleSystemcurrently only logs a message without leveraging themoduleSystemobject. If this is for demonstration or test stubs, it is fine; otherwise, consider applying meaningful initialization logic.
📜 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 (13)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs(10 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(6 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(2 hunks)crates/compiler/tests/fixtures/css/modules/local/output.js(2 hunks)crates/compiler/tests/fixtures/css/modules/normal/output.js(2 hunks)crates/compiler/tests/fixtures/css/prefixer/normal/output.js(2 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/library/external/conflicts/output.js(1 hunks)
🧰 Additional context used
🧬 Code Definitions (11)
crates/compiler/tests/fixtures/css/modules/normal/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/global/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_dynamic_entry_resource_pots.rs (5)
crates/compiler/tests/library.rs (1)
test(13-64)crates/core/src/context/mod.rs (10)
new(58-82)new(222-233)new(334-336)new(348-350)new(362-364)default(186-188)default(305-307)default(340-342)default(354-356)default(368-370)rust-plugins/react/src/lib.rs (2)
new(39-55)name(59-61)crates/core/src/module/meta_data/script.rs (2)
default(102-119)clone(123-152)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (2)
affected_groups(83-91)affected_groups(209-217)
crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/issue_337/output.js (4)
url(69-69)url(75-75)url(83-83)url(140-140)
crates/compiler/tests/fixtures/css/modules/global/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/global/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (4)
crates/core/src/module/module_group.rs (2)
module_group(79-82)to_string(310-312)packages/core/src/types/binding.ts (1)
ModuleType(4-6)crates/core/src/resource/resource_pot.rs (2)
new(60-75)to_string(173-182)crates/core/src/module/mod.rs (2)
to_string(239-244)to_string(365-367)
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (4)
crates/compiler/src/update/diff_and_patch_module_graph.rs (2)
diff_module_graph(91-128)patch_module_graph(131-218)crates/compiler/src/update/patch_module_group_graph.rs (3)
patch_module_group_graph(16-113)module_graph(607-611)module_graph(654-658)crates/compiler/src/update/regenerate_resources/mod.rs (2)
clear_resource_pot_of_modules_in_module_groups(230-245)resource_pot_map(201-205)crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots.rs (3)
generate_and_diff_resource_pots(34-83)resource_pots(268-271)module_groups(91-107)
crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/global/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)
crates/compiler/tests/fixtures/css/prefixer/normal/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/global/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)
crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (17)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/global/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/local/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/issue_337/output.js (4)
url(69-69)url(75-75)url(83-83)url(140-140)
crates/compiler/tests/fixtures/css/modules/local/output.js (16)
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/global/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/prefixer/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/modules/normal/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/partial_bundling/basic/output.js (1)
moduleSystem(23-23)crates/compiler/tests/fixtures/tree_shake/css/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/deep_dynamic_import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/dynamic-import/output.js (1)
moduleSystem(30-30)crates/compiler/tests/fixtures/tree_shake/empty_specifiers/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/non_script/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/basic/output.js (1)
moduleSystem(2-2)crates/compiler/tests/fixtures/tree_shake/changed_ast/output.js (1)
moduleSystem(2-2)
⏰ Context from checks skipped due to timeout of 90000ms (20)
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - android-arm-eabi
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
- GitHub Check: Test - windows-latest
- GitHub Check: Test - ubuntu-latest
- GitHub Check: Test - macos-latest
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-arm64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-arm64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-x64
- GitHub Check: call-rust-build / Build and Upload Artifacts - darwin-arm64
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-gnu
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-x64-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - win32-ia32-msvc
- GitHub Check: call-rust-build / Build and Upload Artifacts - linux-x64-musl
- GitHub Check: call-rust-build / Build and Upload Artifacts - x86_64-unknown-freebsd
🔇 Additional comments (44)
crates/compiler/tests/fixtures/css/prefixer/normal/output.js (4)
1-7: New module system initialization looks goodThe introduction of the module system initialization with a dedicated function is a clean approach. This aligns with the broader efforts to simplify module handling across the codebase.
9-33: CSS structure with browser prefixes is properly maintainedThe CSS keyframes with appropriate vendor prefixes (-webkit, -moz, -o) are correctly preserved, ensuring cross-browser compatibility.
35-40: Module registration approach looks goodThe new module registration implementation uses a clean pattern of importing the runtime first, then iterating through and registering modules with the system.
47-47: Module system bootstrapping looks correctThe entry point resolution approach using the global namespace and module system references is properly implemented.
crates/compiler/tests/fixtures/library/external/conflicts/output.js (7)
1-4: Imports appear well-structured for testing namespace conflicts.The code imports the same function (
existsSync) twice with different aliases, along with full namespace imports of both thefsandmodulemodules. This structure is likely intentional for testing how Farm handles import conflicts and name resolution.
5-6: Variables correctly reference namespace properties.These variable declarations provide direct references to specific functions from the imported namespaces, which is a common pattern for simplifying access to frequently used functions.
7-10: Namespace object creation and export star operation.The code creates a namespace object with the
__esModuleflag and usesdefineExportStarto copy all properties from thenode_module_external_namespace_farm_internal_namespace. This pattern tests how the compiler handles re-exports of entire modules.
11-11: First existsSync usage correctly uses the first alias.The code appropriately uses the first aliased import of
existsSyncto check for a file named 'bar'.
12-16: Second namespace creation with property and export star operation.The code creates another namespace object that includes a reference to the
existsSyncfunction and again usesdefineExportStarto copy properties from the module namespace. This is testing multiple re-exports and property access patterns.
17-20: Multiple existsSync implementations correctly used.The code demonstrates using all three variations of the
existsSyncfunction (both aliases and the namespace property). The comment on line 18 refers to 'readFileSync' but is actually usingexistsSync, which might be a typo, but doesn't affect functionality.Is this comment mismatch ('readFileSync' vs actual 'existsSync' usage) intentional or should it be updated to match the actual function being used?
1-21:❓ Verification inconclusive
Verify window["farm_default_namespace"] availability.
The code depends on
window["__farm_default_namespace__"].defineExportStarbeing defined elsewhere. Ensure this dependency is properly set up in the test environment.Run the following script to check if this method is defined in other test fixture files:
🏁 Script executed:
#!/bin/bash # Search for defineExportStar implementations in test fixtures echo "Searching for defineExportStar implementation..." rg -A 3 "__farm_default_namespace__" --glob "*.{js,ts}" crates/compiler/tests/fixtures/Length of output: 97424
Action Required: Verify the test environment’s setup for farm_default_namespace.defineExportStar
Your code depends on the method
defineExportStaron the global object atwindow["__farm_default_namespace__"]to work correctly. Our search across the test fixtures shows that many files consistently reference properties on this global (for example, using methods like.m,.setPlugins(), or accessing__farm_module_system__), but there was no explicit implementation or mention of adefineExportStarmethod.Please ensure that:
- The test environment provides a definition for
window["__farm_default_namespace__"].defineExportStar(either via a runtime injection or test setup script),- or update the test configuration so that this dependency is properly established before these tests run.
Review and verify that this dependency is available to avoid runtime issues in the fixtures using this API.
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_generate_and_diff_resource_pots.rs (7)
53-70: LGTM! Dynamic entry edge addition looks correctThe code correctly adds module "I" and establishes a dynamic entry edge from "E" to "I", which expands the test case to cover dynamic entry scenarios.
93-108: Good improvement using ModuleGroupId instead of string literalsSwitching from string literals to strongly-typed
ModuleGroupIdimproves type safety and makes the code more maintainable. This change aligns with best practices for Rust's strong typing system.
134-144: LGTM! Resource pot sorting before assertions prevents flaky testsSorting resource pot IDs before assertions ensures consistent ordering for comparison, which prevents potential flaky tests that could fail due to non-deterministic ordering.
146-161: Good addition of GroupResourcePotsSnapshotItem for better test organizationAdding this structure improves test organization and readability by encapsulating related data and providing clear sorting semantics through the Ord implementation.
192-206: Good addition of ModuleResourcePotSnapshotItem for better test organizationSimilar to GroupResourcePotsSnapshotItem, this structure improves test organization and sorting capabilities for module resource pot assertions.
272-276: LGTM! Using assert_resource_pots macro for cleaner assertionsUsing the
assert_resource_pots!macro creates cleaner, more maintainable assertions compared to individual assert statements.
415-503: Good addition of test for module removal scenariosThe new test comprehensively validates the behavior when removing modules from the module graph, which increases test coverage for an important edge case.
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_enforce_resource_pots.rs (4)
37-43: LGTM! Improved module creation with explicit module typeSetting the module type explicitly adds clarity and ensures the correct module type is used. This is a good practice for test code.
73-77: Good improvement using ModuleGroupId instead of string literalsUsing strongly-typed
ModuleGroupIdimproves type safety and makes the code more maintainable.
141-145: Remove or update the commented out assertionThe commented-out assertion appears to be an older test that's no longer relevant, but remains in the codebase.
Either:
- Remove the commented code if it's no longer needed
- Update and uncomment it if it should be part of the test assertions
- Add a comment explaining why it's kept for reference
assert_eq!(enforce_resource_pots, vec!["test__js".to_string()]); - // assert_eq!( - // enforce_resource_pots, - // vec!["test__custom(\"__farm_unknown\")".to_string()] - // );
205-207: LGTM! Consistent usage of ModuleGroupId throughout the codebaseThe code maintains consistency by using ModuleGroupId in both test functions, which is good practice.
crates/compiler/src/update/regenerate_resources/generate_and_diff_resource_pots/test_handle_dynamic_entry_resource_pots.rs (1)
26-158: Well-structured new test for dynamic entry resource potsThis comprehensive test for
handle_dynamic_entry_resource_potsfollows the established testing patterns in the codebase and covers the important functionality:
- It correctly sets up module graphs with dynamic entry relationships
- It verifies affected groups and modules
- It tests the actual dynamic entry resource pot generation
- It asserts on the expected content of the generated resource pots
The test adds good coverage for this functionality, especially the specific dynamic entry use case.
crates/compiler/tests/fixtures/css/url_replacer/ignored/output.js (1)
9-12: Confirm validity of data URI.
The background image is set tourl("data:base64,xfsdfdsa"), which may not be a correct data URI. Verify whether this is a placeholder or if the image data should include a MIME type prefix likedata:image/png;base64,....crates/compiler/tests/fixtures/css/url_replacer/absolute/output.js (2)
9-12: Verify CSS resource path.
url("/font.ttf")references an absolute path. Check whether this location is correct and accessible. If this is a placeholder in a test fixture, you can skip further checks.
14-26: Check module registration flow.
Similar to the previous file, this code registers modules viamoduleSystem.gwithout providing its implementation details. Ensure that the globalwindow["__farm_default_namespace__"]is defined and consistent across tests or production builds.crates/compiler/tests/fixtures/css/modules/composes/local_import/output.js (2)
9-16: Confirm styling changes.
The updated CSS block references larger font size and aredcolor. Confirm it aligns with intended design or test expectations.
18-38: Validate module exports and usage.
Exports map CSS class names ("action","base") to the compiled class strings, and the subsequent lines logfarmRequire.f(_f_index).action. Verify that these references exist and behave as expected.crates/compiler/tests/fixtures/css/modules/normal/output.js (3)
1-7: Streamlined module system initializationThe new implementation introduces a simplified module system approach with cleaner initialization. This is consistent with the breaking changes mentioned in PR #1835 for Farm v2.0, replacing the previous complex interop functions (
_interop_require_default,_export_star, etc.) with a more straightforward initialization.
21-41: Improved module registration and dependency managementThe new implementation uses a more direct approach for module registration and dependency management. The IIFE pattern registers modules with the moduleSystem and sets up the CSS module exports more efficiently.
42-42: Clean module bootstrapping approachThe module bootstrapping has been simplified with direct calls to initialize (
__farm_ms__.b()) and require the entry module (__farm_ms__.r("b5d64806")).crates/compiler/tests/fixtures/css/modules/composes/global_import/output.js (3)
1-7: Consistent module system implementation across fixturesThe module system initialization is correctly implemented in the same way as other fixtures, ensuring consistency throughout the codebase. This standardized approach simplifies maintenance and testing.
18-36: Efficient CSS composition handlingThe implementation properly handles CSS composition with global imports through the module system. The export of
"base": "farm-base action"correctly composes the farm-base class with the action class.
37-37: Consistent bootstrapping patternThe bootstrapping pattern matches other fixtures, establishing a consistent approach throughout the codebase.
crates/compiler/tests/fixtures/css/modules/composes/external_import/output.js (3)
1-7: Consistent module system implementationThe module system initialization follows the same pattern used in other CSS module fixtures, maintaining consistency throughout the codebase.
18-44: Well-implemented external CSS compositionThe implementation elegantly handles external CSS composition across multiple modules. Module "95fe6ac5" correctly imports and composes with module "8b6840d6", demonstrating the new system's capability to handle complex CSS composition scenarios.
45-45: Proper module bootstrappingThe module bootstrapping follows the consistent pattern established throughout the fixtures.
crates/compiler/tests/fixtures/css/modules/global/output.js (2)
1-7: Consistent module system initializationThe module system initialization follows the same pattern used in other CSS module fixtures, maintaining consistency throughout the codebase.
38-38: Consistent bootstrapping patternThe bootstrapping pattern follows the established approach used in other fixtures.
crates/compiler/tests/fixtures/css/modules/local/output.js (5)
3-6: Mismatch between function signature and call parametersThe
initModuleSystemfunction is defined without parameters, but it's being called withmoduleSystemas an argument at line 6. The function should be updated to accept and use this parameter.-function initModuleSystem() { +function initModuleSystem(system) { console.log('module-helper.ts'); }
24-28: LGTM: Module registration logicThe module registration logic correctly iterates through the provided modules and registers each one with the module system using the
gmethod.
30-39: LGTM: CSS module exports structureThe CSS module exports are properly structured, mapping the class names to their transformed values.
40-44: LGTM: Module import and usageThe module correctly imports the CSS module using
farmRequireand accesses thebaseclass through the proper API methods.
46-46: LGTM: Module system bootstrapping and entry point initializationThe module system is properly bootstrapped with
__farm_ms__.b()and the entry point is correctly required using__farm_ms__.r("b5d64806").
| for(var moduleId in modules){ | ||
| var module = modules[moduleId]; | ||
| moduleSystem.g(moduleId, module); | ||
| } | ||
| })(window["__farm_default_namespace__"].m, { | ||
| "95fe6ac5": function(module, exports, farmRequire, farmDynamicRequire) { | ||
| farmRequire._m(exports); | ||
| ""; | ||
| exports.default = {}; | ||
| }, | ||
| "b5d64806": function(module, exports, farmRequire, farmDynamicRequire) { | ||
| farmRequire._m(exports); | ||
| var _f_index = farmRequire.i(farmRequire("95fe6ac5")); | ||
| console.log(farmRequire.f(_f_index).base); | ||
| } |
There was a problem hiding this comment.
Potential issue with accessing undefined property
Module "95fe6ac5" exports an empty object (exports.default = {}), but in module "b5d64806", there's an attempt to access farmRequire.f(_f_index).base which doesn't exist in the empty object. This could lead to a runtime error.
This inconsistency should be addressed by either:
- Adding the "base" property to the exports in module "95fe6ac5"
- Updating the consuming code to handle the case when the property doesn't exist
- exports.default = {};
+ exports.default = {
+ "base": "base"
+ };Or in the consuming module:
- console.log(farmRequire.f(_f_index).base);
+ const styles = farmRequire.f(_f_index);
+ console.log(styles.base || 'base property not found');📝 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.
| import "./farm_internal_runtime_index.js";import "./index-2528a0d0.css";(function(moduleSystem, modules) { | |
| for(var moduleId in modules){ | |
| var module = modules[moduleId]; | |
| moduleSystem.g(moduleId, module); | |
| } | |
| })(window["__farm_default_namespace__"].m, { | |
| "95fe6ac5": function(module, exports, farmRequire, farmDynamicRequire) { | |
| farmRequire._m(exports); | |
| ""; | |
| exports.default = {}; | |
| }, | |
| "b5d64806": function(module, exports, farmRequire, farmDynamicRequire) { | |
| farmRequire._m(exports); | |
| var _f_index = farmRequire.i(farmRequire("95fe6ac5")); | |
| console.log(farmRequire.f(_f_index).base); | |
| } | |
| import "./farm_internal_runtime_index.js"; | |
| import "./index-2528a0d0.css"; | |
| (function(moduleSystem, modules) { | |
| for(var moduleId in modules){ | |
| var module = modules[moduleId]; | |
| moduleSystem.g(moduleId, module); | |
| } | |
| })(window["__farm_default_namespace__"].m, { | |
| "95fe6ac5": function(module, exports, farmRequire, farmDynamicRequire) { | |
| farmRequire._m(exports); | |
| ""; | |
| - exports.default = {}; | |
| + exports.default = { | |
| + "base": "base" | |
| + }; | |
| }, | |
| "b5d64806": function(module, exports, farmRequire, farmDynamicRequire) { | |
| farmRequire._m(exports); | |
| var _f_index = farmRequire.i(farmRequire("95fe6ac5")); | |
| console.log(farmRequire.f(_f_index).base); | |
| } | |
| }); |
* fix: assets_filename_error * fix: resolve server resource use regex split identifier * fix: resolve server resource use regex split identifier
Description:
BREAKING CHANGE:
Related issue (if exists):
#1589
Summary by CodeRabbit
New Features
Refactor
Tests
Chores