Skip to content

Commit

Permalink
fix: duplicate dynamic entries (#380)
Browse files Browse the repository at this point in the history
<!-- Thank you for contributing! -->

### Description

<!-- Please insert your description here and provide especially info about the "what" this PR is solving -->

### Test Plan

<!-- e.g. is there anything you'd like reviewers to focus on? -->

---
  • Loading branch information
hyf0 authored Dec 4, 2023
1 parent 305ddae commit 6067e74
Show file tree
Hide file tree
Showing 12 changed files with 95 additions and 67 deletions.
21 changes: 10 additions & 11 deletions crates/rolldown/src/bundler/chunk/chunk.rs
Original file line number Diff line number Diff line change
@@ -1,19 +1,16 @@
use oxc::span::Atom;
use rolldown_common::{EntryPoint, ModuleId, NamedImport, Specifier, SymbolRef};
use rolldown_common::{EntryPoint, EntryPointKind, ModuleId, NamedImport, Specifier, SymbolRef};
use rustc_hash::FxHashMap;
use string_wizard::{Joiner, JoinerOptions};

use crate::{
bundler::{
bundle::output::RenderedModule,
chunk_graph::ChunkGraph,
module::ModuleRenderContext,
options::{file_name_template::FileNameRenderOptions, output_options::OutputOptions},
stages::link_stage::LinkStageOutput,
bundle::output::RenderedModule, chunk_graph::ChunkGraph, module::ModuleRenderContext,
options::output_options::OutputOptions, stages::link_stage::LinkStageOutput,
utils::bitset::BitSet,
},
error::BatchedResult,
InputOptions,
FileNameTemplate, InputOptions,
};

use super::ChunkId;
Expand Down Expand Up @@ -48,13 +45,15 @@ impl Chunk {
Self { entry_point, modules, name, bits, ..Self::default() }
}

pub fn render_file_name(&mut self, output_options: &OutputOptions) {
let pat = if self.entry_point.is_some() {
pub fn file_name_template<'a>(
&mut self,
output_options: &'a OutputOptions,
) -> &'a FileNameTemplate {
if matches!(self.entry_point, Some(EntryPoint { kind: EntryPointKind::UserDefined, .. })) {
&output_options.entry_file_names
} else {
&output_options.chunk_file_names
};
self.file_name = Some(pat.render(&FileNameRenderOptions { name: self.name.as_deref() }));
}
}

#[allow(clippy::unnecessary_wraps, clippy::cast_possible_truncation)]
Expand Down
8 changes: 4 additions & 4 deletions crates/rolldown/src/bundler/chunk/render_chunk_exports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ impl Chunk {
output_options: &OutputOptions,
) -> Option<MagicString<'static>> {
if let Some(entry) = &self.entry_point {
let linking_info = &graph.linking_infos[entry.module_id];
let linking_info = &graph.linking_infos[entry.id];
if matches!(linking_info.wrap_kind, WrapKind::Cjs) {
match output_options.format {
OutputFormat::Esm => {
Expand All @@ -22,7 +22,7 @@ impl Chunk {
panic!(
"Cannot find canonical name for wrap ref {:?} of {:?}",
linking_info.wrapper_ref.unwrap(),
graph.modules[entry.module_id].resource_id()
graph.modules[entry.id].resource_id()
)
});
return Some(MagicString::new(format!("export default {wrap_ref_name}();\n")));
Expand Down Expand Up @@ -76,7 +76,7 @@ impl Chunk {
tmp
},
|entry_point| {
let linking_info = &graph.linking_infos[entry_point.module_id];
let linking_info = &graph.linking_infos[entry_point.id];
linking_info
.sorted_exports()
.map(|(name, export)| (name.clone(), export.symbol_ref))
Expand All @@ -91,7 +91,7 @@ impl Chunk {
output_options: &OutputOptions,
) -> Vec<String> {
if let Some(entry_point) = &self.entry_point {
let linking_info = &graph.linking_infos[entry_point.module_id];
let linking_info = &graph.linking_infos[entry_point.id];
if matches!(linking_info.wrap_kind, WrapKind::Cjs) {
match output_options.format {
OutputFormat::Esm => {
Expand Down
2 changes: 1 addition & 1 deletion crates/rolldown/src/bundler/module/normal_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ use super::{Module, ModuleRenderContext, ModuleVec};
pub struct NormalModule {
pub exec_order: u32,
pub id: ModuleId,
pub is_entry: bool,
pub is_user_defined_entry: bool,
pub resource_id: ResourceId,
pub pretty_path: String,
/// Representative name of `FilePath`, which is created by `FilePath#representative_name` belong to `resource_id`
Expand Down
4 changes: 2 additions & 2 deletions crates/rolldown/src/bundler/module/normal_module_builder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ pub struct NormalModuleBuilder {
pub namespace_symbol: Option<SymbolRef>,
pub exports_kind: Option<ExportsKind>,
pub module_type: ModuleType,
pub is_entry: bool,
pub is_user_defined_entry: Option<bool>,
pub pretty_path: Option<String>,
}

Expand All @@ -54,7 +54,7 @@ impl NormalModuleBuilder {
namespace_symbol: self.namespace_symbol.unwrap(),
exports_kind: self.exports_kind.unwrap_or(ExportsKind::Esm),
module_type: self.module_type,
is_entry: self.is_entry,
is_user_defined_entry: self.is_user_defined_entry.unwrap(),
pretty_path: self.pretty_path.unwrap(),
}
}
Expand Down
61 changes: 36 additions & 25 deletions crates/rolldown/src/bundler/module_loader/module_loader.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ pub struct ModuleLoaderOutput {
pub modules: ModuleVec,
pub symbols: Symbols,
// Entries that user defined + dynamic import entries
pub entries: Vec<EntryPoint>,
pub entry_points: Vec<EntryPoint>,
pub runtime: RuntimeModuleBrief,
pub warnings: Vec<BuildError>,
}
Expand Down Expand Up @@ -83,14 +83,14 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {
id
}

fn try_spawn_new_task(&mut self, info: &ResolvedRequestInfo, is_entry: bool) -> ModuleId {
fn try_spawn_new_task(&mut self, info: ResolvedRequestInfo) -> ModuleId {
match self.visited.entry(info.path.clone()) {
std::collections::hash_map::Entry::Occupied(visited) => *visited.get(),
std::collections::hash_map::Entry::Vacant(not_visited) => {
let id = Self::alloc_module_id(&mut self.intermediate_modules, &mut self.symbols);
not_visited.insert(id);
if info.is_external {
let ext = ExternalModule::new(id, ResourceId::new(info.path.clone()));
let ext = ExternalModule::new(id, ResourceId::new(info.path));
self.intermediate_modules[id] = Some(Module::External(ext));
} else {
self.remaining += 1;
Expand All @@ -101,7 +101,6 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {
// safety: Data in `ModuleTaskContext` are alive as long as the `NormalModuleTask`, but rustc doesn't know that.
unsafe { self.common_data.assume_static() },
id,
is_entry,
module_path,
info.module_type,
);
Expand All @@ -124,7 +123,7 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {

pub async fn fetch_all_modules(
mut self,
user_defined_entries: &[(Option<String>, ResolvedRequestInfo)],
user_defined_entries: Vec<(Option<String>, ResolvedRequestInfo)>,
) -> BatchedResult<ModuleLoaderOutput> {
assert!(!self.input_options.input.is_empty(), "You must supply options.input to rolldown");

Expand All @@ -134,20 +133,25 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {
self.intermediate_modules.reserve(user_defined_entries.len() + 1 /* runtime */);

// Store the already consider as entry module
let mut entry_module_set = FxHashSet::default();
let mut entries = user_defined_entries
.iter()
let mut user_defined_entry_ids = {
let mut tmp = FxHashSet::default();
tmp.reserve(user_defined_entries.len());
tmp
};

let mut entry_points = user_defined_entries
.into_iter()
.map(|(name, info)| EntryPoint {
name: name.clone(),
module_id: self.try_spawn_new_task(info, true),
kind: EntryPointKind::UserSpecified,
name,
id: self.try_spawn_new_task(info),
kind: EntryPointKind::UserDefined,
})
.inspect(|e| {
entry_module_set.insert(e.module_id);
user_defined_entry_ids.insert(e.id);
})
.collect::<Vec<_>>();

let mut dynamic_entries = vec![];
let mut dynamic_import_entry_ids = FxHashSet::default();

let mut runtime_brief: Option<RuntimeModuleBrief> = None;

Expand All @@ -170,19 +174,18 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {
.into_iter()
.zip(resolved_deps)
.map(|(raw_rec, info)| {
let id = self.try_spawn_new_task(&info, false);
// dynamic import as extra entries if enable code splitting
if raw_rec.kind == ImportKind::DynamicImport && !entry_module_set.contains(&id) {
dynamic_entries.push(EntryPoint {
name: Some(info.path.unique(&self.input_options.cwd)),
module_id: id,
kind: EntryPointKind::DynamicImport,
});
let id = self.try_spawn_new_task(info);
// Dynamic imported module will be considered as an entry
if matches!(raw_rec.kind, ImportKind::DynamicImport)
&& !user_defined_entry_ids.contains(&id)
{
dynamic_import_entry_ids.insert(id);
}
raw_rec.into_import_record(id)
})
.collect::<IndexVec<ImportRecordId, _>>();
builder.import_records = Some(import_records);
builder.is_user_defined_entry = Some(user_defined_entry_ids.contains(&module_id));
self.intermediate_modules[module_id] = Some(Module::Normal(builder.build()));

self.symbols.add_ast_symbol(module_id, ast_symbol);
Expand All @@ -206,14 +209,22 @@ impl<T: FileSystem + 'static + Default> ModuleLoader<T> {
return Err(errors);
}

let modules = self.intermediate_modules.into_iter().map(Option::unwrap).collect();
let modules: IndexVec<ModuleId, Module> =
self.intermediate_modules.into_iter().map(Option::unwrap).collect();

let mut dynamic_import_entry_ids = dynamic_import_entry_ids.into_iter().collect::<Vec<_>>();
dynamic_import_entry_ids.sort_by_key(|id| modules[*id].resource_id());

entry_points.extend(dynamic_import_entry_ids.into_iter().map(|id| EntryPoint {
name: None,
id,
kind: EntryPointKind::DynamicImport,
}));

dynamic_entries.sort_unstable_by(|a, b| a.name.cmp(&b.name));
entries.extend(dynamic_entries);
Ok(ModuleLoaderOutput {
modules,
symbols: self.symbols,
entries,
entry_points,
runtime: runtime_brief.expect("Failed to find runtime module. This should not happen"),
warnings: all_warnings,
})
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,18 +35,16 @@ pub struct NormalModuleTask<'task, T: FileSystem + Default> {
module_id: ModuleId,
path: FilePath,
module_type: ModuleType,
is_entry: bool,
}

impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
pub fn new(
ctx: &'task ModuleTaskCommonData<T>,
id: ModuleId,
is_entry: bool,
path: FilePath,
module_type: ModuleType,
) -> Self {
Self { ctx, module_id: id, is_entry, path, module_type }
Self { ctx, module_id: id, path, module_type }
}
pub async fn run(mut self) {
if let Err(errs) = self.run_inner().await {
Expand Down Expand Up @@ -98,7 +96,6 @@ impl<'task, T: FileSystem + Default + 'static> NormalModuleTask<'task, T> {
exports_kind: Some(exports_kind),
namespace_symbol: Some(namespace_symbol),
module_type: self.module_type,
is_entry: self.is_entry,
pretty_path: Some(self.path.prettify(&self.ctx.input_options.cwd)),
..Default::default()
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ impl RuntimeNormalModuleTask {
builder.exports_kind = Some(ExportsKind::Esm);
builder.namespace_symbol = Some(namespace_symbol);
builder.pretty_path = Some("<runtime>".to_string());
builder.is_user_defined_entry = Some(false);

self
.tx
Expand Down
40 changes: 30 additions & 10 deletions crates/rolldown/src/bundler/stages/bundle_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::{
},
chunk_graph::ChunkGraph,
module::Module,
options::output_options::OutputOptions,
options::{file_name_template::FileNameRenderOptions, output_options::OutputOptions},
stages::link_stage::LinkStageOutput,
utils::bitset::BitSet,
},
Expand Down Expand Up @@ -39,11 +39,7 @@ impl<'a> BundleStage<'a> {
use rayon::prelude::*;
let mut chunk_graph = self.generate_chunks();

chunk_graph
.chunks
.iter_mut()
.par_bridge()
.for_each(|chunk| chunk.render_file_name(self.output_options));
self.generate_chunk_filenames(&mut chunk_graph);

self.compute_cross_chunk_links(&mut chunk_graph);

Expand All @@ -63,10 +59,10 @@ impl<'a> BundleStage<'a> {
Output::Chunk(Box::new(OutputChunk {
file_name: c.file_name.clone().unwrap(),
code: content,
is_entry: matches!(&c.entry_point, Some(e) if e.kind == EntryPointKind::UserSpecified),
is_entry: matches!(&c.entry_point, Some(e) if e.kind == EntryPointKind::UserDefined),
is_dynamic_entry: matches!(&c.entry_point, Some(e) if e.kind == EntryPointKind::DynamicImport),
facade_module_id: c.entry_point.as_ref().map(|entry_point| {
self.link_output.modules[entry_point.module_id].expect_normal().pretty_path.to_string()
self.link_output.modules[entry_point.id].expect_normal().pretty_path.to_string()
}),
modules: rendered_modules,
exports: c.get_export_names(self.link_output, self.output_options),
Expand Down Expand Up @@ -165,7 +161,7 @@ impl<'a> BundleStage<'a> {
}

if let Some(entry_point) = &chunk.entry_point {
let entry_module = &self.link_output.modules[entry_point.module_id];
let entry_module = &self.link_output.modules[entry_point.id];
let Module::Normal(entry_module) = entry_module else {
return;
};
Expand Down Expand Up @@ -294,7 +290,7 @@ impl<'a> BundleStage<'a> {
);

self.determine_reachable_modules_for_entry(
entry_point.module_id,
entry_point.id,
i.try_into().unwrap(),
&mut module_to_bits,
);
Expand Down Expand Up @@ -329,4 +325,28 @@ impl<'a> BundleStage<'a> {

ChunkGraph { chunks, module_to_chunk }
}

fn generate_chunk_filenames(&self, chunk_graph: &mut ChunkGraph) {
chunk_graph.chunks.iter_mut().for_each(|chunk| {
let file_name_tmp = chunk.file_name_template(self.output_options);
let chunk_name = chunk.name.clone().unwrap_or_else(|| {
let module_id = if let Some(entry_point) = &chunk.entry_point {
debug_assert!(
matches!(entry_point.kind, EntryPointKind::DynamicImport),
"User-defined entry point should always have a name"
);
entry_point.id
} else {
// TODO: we currently use the first executed module to calculate the chunk name for common chunks
// This is not perfect, should investigate more to find a better solution
chunk.modules.first().copied().unwrap()
};
let module = &self.link_output.modules[module_id];
module.resource_id().expect_file().unique(&self.input_options.cwd)
});

chunk.file_name =
Some(file_name_tmp.render(&FileNameRenderOptions { name: Some(&chunk_name) }));
});
}
}
6 changes: 3 additions & 3 deletions crates/rolldown/src/bundler/stages/link_stage.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ impl LinkStage {
.map(|_| LinkingInfo::default())
.collect::<IndexVec<ModuleId, _>>(),
modules: scan_stage_output.modules,
entries: scan_stage_output.entries,
entries: scan_stage_output.entry_points,
symbols: scan_stage_output.symbols,
runtime: scan_stage_output.runtime,
warnings: scan_stage_output.warnings,
Expand Down Expand Up @@ -93,7 +93,7 @@ impl LinkStage {
let mut stack = self
.entries
.iter()
.map(|entry_point| Action::Enter(entry_point.module_id))
.map(|entry_point| Action::Enter(entry_point.id))
.rev()
.collect::<Vec<_>>();
// The runtime module should always be the first module to be executed
Expand Down Expand Up @@ -309,7 +309,7 @@ impl LinkStage {
include_statement(context, module, stmt_info_id);
}
});
if module.is_entry {
if module.is_user_defined_entry {
let linking_info = &self.linking_infos[module.id];
linking_info.resolved_exports.values().for_each(|resolved_export| {
include_symbol(context, resolved_export.symbol_ref);
Expand Down
Loading

0 comments on commit 6067e74

Please sign in to comment.