fix(turbopack): merged alias lost module factory for missing type import#121
fix(turbopack): merged alias lost module factory for missing type import#121fireairforce merged 1 commit intoutoofrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue in Turbopack where merged aliases were losing their module factories, particularly affecting import namespaces due to incorrect scope hoisting and missing export type bindings. The changes introduce a more robust module merging process that correctly identifies and exposes module factories for these cases, ensuring that bundled modules behave as expected and preventing runtime errors related to missing module definitions. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request appears to fix an issue in Turbopack's scope hoisting where a module factory was being lost. This is achieved by passing around fallback_import_idents from the module merging step and using this information to correctly identify all modules that require a module factory. The logic seems sound. I've included one suggestion to optimize a loop by avoiding unnecessary object creation, which should provide a minor performance benefit. The other change in transform/mod.rs appears to be a necessary code cleanup, possibly due to a dependency update, and looks correct.
| if module == first_entry { | ||
| return None; | ||
| } | ||
| let fallback_ident: Atom = | ||
| crate::magic_identifier::mangle(&format!("imported module {module_id}")) | ||
| .into(); | ||
| if exposure == MergeableModuleExposure::External | ||
| || fallback_import_idents.contains(&fallback_ident) | ||
| { | ||
| Some(module_id) | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
For a minor performance improvement, you can avoid the cost of creating fallback_ident (which involves string formatting, mangling, and atom interning) when exposure == MergeableModuleExposure::External is true. By checking for this condition first and returning early, you can skip the unnecessary work inside this filter_map closure.
if module == first_entry {
return None;
}
if exposure == MergeableModuleExposure::External {
return Some(module_id);
}
let fallback_ident: Atom =
crate::magic_identifier::mangle(&format!("imported module {module_id}"))
.into();
if fallback_import_idents.contains(&fallback_ident) {
Some(module_id)
} else {
None
}
fix scope hoisting with missing export type binding cause module factory lost: