-
Notifications
You must be signed in to change notification settings - Fork 26.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
perf(turbopack): Do not create module part per a star re-export #70383
base: kdy1/ts-action-with-client-proxy
Are you sure you want to change the base?
Changes from all commits
3cd345e
8ec6c2f
ff6bb0d
86bc05e
727013a
bd619f9
bb3ba22
5ec9b8e
6265fe1
7b1982a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -200,8 +200,43 @@ pub(crate) enum ImportedSymbol { | |
ModuleEvaluation, | ||
Symbol(JsWord), | ||
Exports, | ||
/// We need a separate variant for `export * from 'package-star'`. | ||
/// | ||
/// `package-star/index.js`: | ||
/// | ||
/// ``` | ||
/// export { notCompiled } from "./not-compiled.js"; | ||
/// export { notExisting } from "./not-existing.js"; | ||
/// export { notExecuted } from "./not-executed.js"; | ||
/// export * from "./not-executed.js"; | ||
/// export * from "./a.js"; | ||
/// export * from "./b.js"; | ||
/// export const local = "local"; | ||
/// ``` | ||
/// | ||
/// | ||
/// `package-reexport/index.js`: | ||
/// | ||
/// ``` | ||
/// export * from "package-star"; | ||
/// export const outer = "outer"; | ||
/// ``` | ||
/// | ||
/// `import { a } from 'package-reexport'` currently creates `ModulePart::Export("a")` for | ||
/// `package-reexport` and `ModulePart::Exports` for `package-star`. | ||
/// | ||
/// To make side effect optimization work, we need to create `ModulePart::Export("a")`for | ||
/// `export * from "package-star"`. | ||
/// | ||
/// But we cannot make `ImportAnalyzer` or `EvalContext` take the name of the target symbol, | ||
/// because it will make them parameterized by the name of the target symbol and it's too bad | ||
/// for caching. | ||
/// | ||
/// So we need to have a separate variant, and reuse the requested `ModulePart` for reexports. | ||
ReexportAll, | ||
Part(u32), | ||
PartEvaluation(u32), | ||
StarReexports, | ||
} | ||
|
||
#[derive(Debug, Clone, PartialEq, Eq, Hash)] | ||
|
@@ -402,7 +437,7 @@ impl Visit for Analyzer<'_> { | |
let i = self.ensure_reference( | ||
export.span, | ||
export.src.value.clone(), | ||
symbol.unwrap_or(ImportedSymbol::Exports), | ||
symbol.unwrap_or(ImportedSymbol::ReexportAll), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That looks wrong to me. When I |
||
annotations, | ||
); | ||
if let Some(i) = i { | ||
|
@@ -586,12 +621,15 @@ pub(crate) fn orig_name(n: &ModuleExportName) -> JsWord { | |
} | ||
|
||
fn parse_with(with: Option<&ObjectLit>) -> Option<ImportedSymbol> { | ||
find_turbopack_part_id_in_asserts(with?).map(|v| match v { | ||
let id = find_turbopack_part_id_in_asserts(with?)?; | ||
|
||
Some(match id { | ||
PartId::Internal(index, true) => ImportedSymbol::PartEvaluation(index), | ||
PartId::Internal(index, false) => ImportedSymbol::Part(index), | ||
PartId::ModuleEvaluation => ImportedSymbol::ModuleEvaluation, | ||
PartId::Export(e) => ImportedSymbol::Symbol(e.as_str().into()), | ||
PartId::Exports => ImportedSymbol::Exports, | ||
PartId::StarReexports => ImportedSymbol::StarReexports, | ||
}) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -104,8 +104,24 @@ impl Module for EcmascriptModuleFacadeModule { | |
self.module, | ||
ModulePart::locals(), | ||
))); | ||
references.push(Vc::upcast(EcmascriptModulePartReference::new_part( | ||
self.module, | ||
ModulePart::star_reexports(), | ||
))); | ||
references | ||
} | ||
ModulePart::StarReexports { .. } => { | ||
let Some(module) = | ||
Vc::try_resolve_sidecast::<Box<dyn EcmascriptAnalyzable>>(self.module).await? | ||
else { | ||
bail!( | ||
"Expected EcmascriptModuleAsset for a EcmascriptModuleFacadeModule with \ | ||
ModulePart::Evaluation" | ||
); | ||
}; | ||
let result = module.analyze().await?; | ||
result.reexport_references.await?.clone_value() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that only star reexports or all reexports? |
||
} | ||
ModulePart::Facade => { | ||
vec![ | ||
Vc::upcast(EcmascriptModulePartReference::new_part( | ||
|
@@ -194,6 +210,16 @@ impl EcmascriptChunkPlaceable for EcmascriptModuleFacadeModule { | |
} | ||
star_exports.extend(esm_exports.star_exports.iter().copied()); | ||
} | ||
ModulePart::StarReexports => { | ||
let EcmascriptExports::EsmExports(esm_exports) = *self.module.get_exports().await? | ||
else { | ||
bail!( | ||
"EcmascriptModuleFacadeModule must only be used on modules with EsmExports" | ||
); | ||
}; | ||
let esm_exports = esm_exports.await?; | ||
star_exports.extend(esm_exports.star_exports.iter().copied()); | ||
} | ||
ModulePart::Facade => { | ||
// Reexport everything from the reexports module | ||
// (including default export if any) | ||
|
@@ -268,6 +294,7 @@ impl EcmascriptChunkPlaceable for EcmascriptModuleFacadeModule { | |
.module | ||
.is_marked_as_side_effect_free(side_effect_free_packages), | ||
ModulePart::Exports | ||
| ModulePart::StarReexports | ||
| ModulePart::RenamedExport { .. } | ||
| ModulePart::RenamedNamespace { .. } => Vc::cell(true), | ||
_ => bail!("Unexpected ModulePart for EcmascriptModuleFacadeModule"), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need any special fragments or that kind of thing. You need to have a logic to follow reexports in
turbopack/lib.rs
as it is used for the side effects optimization