Skip to content

Commit

Permalink
Small tree shaking fix and test case update (vercel/turborepo#8347)
Browse files Browse the repository at this point in the history
### Description

avoid adding module evaluation to internal part imports
add a broken test case
update split-chunk test case to work and be more clear

### Testing Instructions

<!--
  Give a quick description of steps to test your changes.
-->

---------

Co-authored-by: 강동윤 (Donny) <kdy1997.dev@gmail.com>
  • Loading branch information
sokra and kdy1 authored Jun 7, 2024
1 parent 393d5cc commit b78120b
Show file tree
Hide file tree
Showing 29 changed files with 509 additions and 694 deletions.
58 changes: 28 additions & 30 deletions crates/turbopack-ecmascript/src/analyzer/imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -268,15 +268,21 @@ impl Visit for Analyzer<'_> {
fn visit_import_decl(&mut self, import: &ImportDecl) {
let annotations = ImportAnnotations::parse(import.with.as_deref());

self.ensure_reference(
import.span,
import.src.value.clone(),
ImportedSymbol::ModuleEvaluation,
annotations.clone(),
);
let internal_symbol = parse_with(import.with.as_deref());

if internal_symbol.is_none() || import.specifiers.is_empty() {
self.ensure_reference(
import.span,
import.src.value.clone(),
ImportedSymbol::ModuleEvaluation,
annotations.clone(),
);
}

for s in &import.specifiers {
let symbol = get_import_symbol_from_import(s, import.with.as_deref());
let symbol = internal_symbol
.clone()
.unwrap_or_else(|| get_import_symbol_from_import(s));
let i = self.ensure_reference(
import.span,
import.src.value.clone(),
Expand Down Expand Up @@ -339,15 +345,21 @@ impl Visit for Analyzer<'_> {

let annotations = ImportAnnotations::parse(export.with.as_deref());

self.ensure_reference(
export.span,
src.value.clone(),
ImportedSymbol::ModuleEvaluation,
annotations.clone(),
);
let internal_symbol = parse_with(export.with.as_deref());

if internal_symbol.is_none() || export.specifiers.is_empty() {
self.ensure_reference(
export.span,
src.value.clone(),
ImportedSymbol::ModuleEvaluation,
annotations.clone(),
);
}

for spec in export.specifiers.iter() {
let symbol = get_import_symbol_from_export(spec, export.with.as_deref());
let symbol = internal_symbol
.clone()
.unwrap_or_else(|| get_import_symbol_from_export(spec));

let i =
self.ensure_reference(export.span, src.value.clone(), symbol, annotations.clone());
Expand Down Expand Up @@ -423,14 +435,7 @@ fn parse_with(with: Option<&ObjectLit>) -> Option<ImportedSymbol> {
})
}

fn get_import_symbol_from_import(
specifier: &ImportSpecifier,
with: Option<&ObjectLit>,
) -> ImportedSymbol {
if let Some(part) = parse_with(with) {
return part;
}

fn get_import_symbol_from_import(specifier: &ImportSpecifier) -> ImportedSymbol {
match specifier {
ImportSpecifier::Named(ImportNamedSpecifier {
local, imported, ..
Expand All @@ -443,14 +448,7 @@ fn get_import_symbol_from_import(
}
}

fn get_import_symbol_from_export(
specifier: &ExportSpecifier,
with: Option<&ObjectLit>,
) -> ImportedSymbol {
if let Some(part) = parse_with(with) {
return part;
}

fn get_import_symbol_from_export(specifier: &ExportSpecifier) -> ImportedSymbol {
match specifier {
ExportSpecifier::Named(ExportNamedSpecifier { orig, .. }) => {
ImportedSymbol::Symbol(orig_name(orig))
Expand Down
14 changes: 0 additions & 14 deletions crates/turbopack-ecmascript/src/references/esm/base.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ use turbopack_core::{
ChunkItemExt, ChunkableModule, ChunkableModuleReference, ChunkingContext, ChunkingType,
ChunkingTypeOption, ModuleId,
},
context::AssetContext,
issue::{IssueSeverity, IssueSource},
module::Module,
reference::ModuleReference,
Expand Down Expand Up @@ -152,7 +151,6 @@ impl ModuleReference for EsmAssetReference {
EcmaScriptModulesReferenceSubType::Import
};

// Skip side effect free self-references here.
if let Request::Module { module, .. } = &*self.request.await? {
if module == TURBOPACK_PART_IMPORT_SOURCE {
if let Some(part) = self.export_name {
Expand All @@ -161,18 +159,6 @@ impl ModuleReference for EsmAssetReference {
.await?
.expect("EsmAssetReference origin should be a EcmascriptModuleAsset");

let side_effect_free_packages =
full_module.asset_context().side_effect_free_packages();

if let ModulePart::Evaluation = *part.await? {
if *full_module
.is_marked_as_side_effect_free(side_effect_free_packages)
.await?
{
return Ok(ModuleResolveResult::ignored().cell());
}
}

let module =
EcmascriptModulePartAsset::new(full_module, part, self.import_externals);

Expand Down
4 changes: 2 additions & 2 deletions crates/turbopack-ecmascript/src/tree_shake/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ impl DepGraph {
let (orig, mut local, exported) = match s {
ExportSpecifier::Named(s) => (
Some(s.orig.clone()),
match s.exported.as_ref().unwrap_or(&s.orig) {
match &s.orig {
ModuleExportName::Ident(i) => i.clone(),
ModuleExportName::Str(..) => quote_ident!("_tmp"),
},
Expand Down Expand Up @@ -1006,7 +1006,7 @@ impl DepGraph {
ModuleItem::ModuleDecl(
ModuleDecl::ExportDefaultDecl(..)
| ModuleDecl::ExportDefaultExpr(..)
| ModuleDecl::ExportNamed(NamedExport { src: Some(..), .. }),
| ModuleDecl::ExportNamed(NamedExport { .. }),
) => {}

_ => {
Expand Down
Loading

0 comments on commit b78120b

Please sign in to comment.