Skip to content
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

fix: incremental make rebuild removed entry module #9209

Merged
merged 3 commits into from
Feb 9, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -187,7 +187,7 @@ pub async fn recovery_module_graph(
if module_exist {
None
} else {
mg.revoke_connection(dep_id, false)
mg.revoke_dependency(dep_id, false)
}
})
.collect::<HashSet<_>>();
Expand Down
1 change: 0 additions & 1 deletion crates/rspack_core/src/compiler/compilation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -821,7 +821,6 @@ impl Compilation {
#[instrument("Compilation:make", skip_all)]
pub async fn make(&mut self) -> Result<()> {
self.make_artifact.reset_dependencies_incremental_info();
// self.module_executor.
// run module_executor
if let Some(module_executor) = &mut self.module_executor {
let mut module_executor = std::mem::take(module_executor);
Expand Down
95 changes: 57 additions & 38 deletions crates/rspack_core/src/compiler/make/cutout/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ mod fix_build_meta;
mod fix_issuers;
mod has_module_graph_change;

use rspack_collections::IdentifierSet;
use rspack_collections::{IdentifierSet, UkeySet};
use rustc_hash::FxHashSet as HashSet;

use self::{
Expand All @@ -27,33 +27,22 @@ impl Cutout {
artifact: &mut MakeArtifact,
params: Vec<MakeParam>,
) -> HashSet<BuildDependency> {
let mut entry_dependencies = std::mem::take(&mut artifact.entry_dependencies);
let mut next_entry_dependencies = HashSet::default();
let mut clean_entry_dependencies = false;
let mut useless_entry_dependencies = UkeySet::default();
let mut force_build_modules = IdentifierSet::default();
let mut force_build_deps = HashSet::default();
let mut remove_entry_deps = HashSet::default();

let module_graph = artifact.get_module_graph();

for item in params {
match item {
MakeParam::BuildEntry(deps) => {
for dep_id in deps {
if !entry_dependencies.contains(&dep_id) {
force_build_deps.insert((dep_id, None));
entry_dependencies.insert(dep_id);
}
}
next_entry_dependencies = deps;
}
MakeParam::BuildEntryAndClean(deps) => {
remove_entry_deps.extend(std::mem::take(&mut entry_dependencies));
entry_dependencies = deps;
for dep_id in &entry_dependencies {
if remove_entry_deps.contains(dep_id) {
remove_entry_deps.remove(dep_id);
} else {
force_build_deps.insert((*dep_id, None));
}
}
next_entry_dependencies = deps;
clean_entry_dependencies = true;
}
MakeParam::CheckNeedBuild => {
force_build_modules.extend(module_graph.modules().values().filter_map(|module| {
Expand All @@ -65,32 +54,30 @@ impl Cutout {
}));
}
MakeParam::ModifiedFiles(files) => {
force_build_modules.extend(module_graph.modules().values().filter_map(|module| {
for module in module_graph.modules().values() {
// check has dependencies modified
if module.depends_on(&files) {
Some(module.identifier())
} else {
None
// add module id
force_build_modules.insert(module.identifier());
}
}))
}
}
MakeParam::RemovedFiles(files) => {
force_build_modules.extend(module_graph.modules().values().flat_map(|module| {
let mut res = vec![];

for module in module_graph.modules().values() {
// check has dependencies modified
if module.depends_on(&files) {
// add module id
res.push(module.identifier());
// add parent module id
res.extend(
module_graph
.get_incoming_connections(&module.identifier())
.filter_map(|connect| connect.original_module_identifier),
)
force_build_modules.insert(module.identifier());
// process parent module id
for connect in module_graph.get_incoming_connections(&module.identifier()) {
if let Some(original_module_identifier) = connect.original_module_identifier {
force_build_modules.insert(original_module_identifier);
} else {
useless_entry_dependencies.insert(connect.dependency_id);
}
}
}
res
}))
}
}
MakeParam::ForceBuildDeps(deps) => {
for item in deps {
Expand Down Expand Up @@ -128,23 +115,55 @@ impl Cutout {
force_build_deps.extend(artifact.revoke_module(&id));
}

let mut entry_dependencies = std::mem::take(&mut artifact.entry_dependencies);
let mut module_graph = artifact.get_module_graph_mut();
for dep_id in remove_entry_deps {
// remove useless entry dependencies
let mut remove_entry_dependencies = HashSet::default();
for dep_id in useless_entry_dependencies {
if !next_entry_dependencies.contains(&dep_id) {
remove_entry_dependencies.insert(dep_id);
}
}
if clean_entry_dependencies {
remove_entry_dependencies.extend(entry_dependencies.difference(&next_entry_dependencies));
}
for dep_id in remove_entry_dependencies {
// connection may have been deleted by revoke module
if let Some(con) = module_graph.connection_by_dependency_id(&dep_id) {
// need clean_isolated_module to check whether the module is still used by other deps
self
.clean_isolated_module
.add_need_check_module(*con.module_identifier());
module_graph.revoke_connection(&dep_id, true);
}
force_build_deps.remove(&(dep_id, None));
module_graph.revoke_dependency(&dep_id, true);
entry_dependencies.remove(&dep_id);
}

// add entry dependencies
for dep in next_entry_dependencies
.difference(&entry_dependencies)
.copied()
.collect::<Vec<_>>()
{
force_build_deps.insert((dep, None));
entry_dependencies.insert(dep);
}

artifact.entry_dependencies = entry_dependencies;

self.has_module_graph_change.analyze_artifact(artifact);

// only return available force_build_deps
let module_graph = artifact.get_module_graph();
force_build_deps
.into_iter()
.filter(|(dep_id, _)| {
let Some(dep) = module_graph.dependency_by_id(dep_id) else {
return false;
};
dep.as_module_dependency().is_some() || dep.as_context_dependency().is_some()
})
.collect()
}

pub fn fix_artifact(self, artifact: &mut MakeArtifact) {
Expand Down
15 changes: 3 additions & 12 deletions crates/rspack_core/src/compiler/make/repair/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -102,22 +102,13 @@ pub async fn repair(
let module_graph = artifact.get_module_graph_mut();
let init_tasks = build_dependencies
.into_iter()
.filter_map::<Box<dyn Task<MakeTaskContext>>, _>(|(id, parent_module_identifier)| {
.map::<Box<dyn Task<MakeTaskContext>>, _>(|(id, parent_module_identifier)| {
let dependency = module_graph
.dependency_by_id(&id)
.expect("dependency not found");
// filter module_dependency and context_dependency
if dependency.as_module_dependency().is_none() && dependency.as_context_dependency().is_none()
{
return None;
}

// filter parent module existed dependency
let parent_module =
parent_module_identifier.and_then(|id| module_graph.module_by_identifier(&id));
if parent_module_identifier.is_some() && parent_module.is_none() {
return None;
}

let current_profile = compilation
.options
Expand All @@ -134,7 +125,7 @@ pub async fn repair(
None
}
});
Some(Box::new(factorize::FactorizeTask {
Box::new(factorize::FactorizeTask {
compilation_id: compilation.id(),
module_factory: compilation.get_dependency_factory(dependency),
original_module_identifier: parent_module_identifier,
Expand All @@ -149,7 +140,7 @@ pub async fn repair(
options: compilation.options.clone(),
current_profile,
resolver_factory: compilation.resolver_factory.clone(),
}))
})
})
.collect::<Vec<_>>();

Expand Down
32 changes: 19 additions & 13 deletions crates/rspack_core/src/module_graph/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -213,35 +213,41 @@ impl<'a> ModuleGraph<'a> {
map
}

/// Remove a connection and return connection origin module identifier and dependency
/// Remove dependency in mgm and target connection, return dependency_id and origin module identifier
///
/// force will completely remove dependency, and you will not regenerate it from dependency_id
pub fn revoke_connection(
pub fn revoke_dependency(
&mut self,
dep_id: &DependencyId,
force: bool,
) -> Option<BuildDependency> {
let connection = self.connection_by_dependency_id(dep_id)?;
let module_identifier = *connection.module_identifier();
let original_module_identifier = connection.original_module_identifier;
let connection_info = self.connection_by_dependency_id(dep_id).map(|connection| {
(
connection.original_module_identifier,
*connection.module_identifier(),
)
});

let Some(active_partial) = &mut self.active else {
panic!("should have active partial");
};
active_partial.connections.insert(*dep_id, None);
if connection_info.is_some() {
active_partial.connections.insert(*dep_id, None);
}
if force {
active_partial.dependencies.insert(*dep_id, None);
active_partial
.dependency_id_to_parents
.insert(*dep_id, None);
}

let (original_module_identifier, module_identifier) = connection_info?;
// remove outgoing from original module graph module
if let Some(original_module_identifier) = &original_module_identifier {
if let Some(mgm) = self.module_graph_module_by_identifier_mut(original_module_identifier) {
mgm.remove_outgoing_connection(dep_id);
// Because of mgm.dependencies is set when original module build success
// it does not need to remove dependency in mgm.dependencies.
// Because of mgm.all_dependencies is set when original module build success
// it does not need to remove dependency in mgm.all_dependencies.
}
}
// remove incoming from module graph module
Expand All @@ -258,12 +264,12 @@ impl<'a> ModuleGraph<'a> {
.map(|m| Vec::from(m.get_blocks()))
.unwrap_or_default();

let (outgoing_connections, incoming_connections) = self
let (incoming_connections, all_dependencies) = self
.module_graph_module_by_identifier(module_id)
.map(|mgm| {
(
mgm.outgoing_connections().clone(),
mgm.incoming_connections().clone(),
mgm.all_dependencies.clone(),
)
})
.unwrap_or_default();
Expand All @@ -279,13 +285,13 @@ impl<'a> ModuleGraph<'a> {
active_partial.blocks.insert(block, None);
}

for cid in outgoing_connections {
self.revoke_connection(&cid, true);
for dep_id in all_dependencies {
self.revoke_dependency(&dep_id, true);
}

incoming_connections
.iter()
.filter_map(|cid| self.revoke_connection(cid, false))
.filter_map(|dep_id| self.revoke_dependency(dep_id, false))
.collect()
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ async fn finish_modules(&self, compilation: &mut Compilation) -> Result<()> {
let boxed_dep = Box::new(new_dep.clone()) as BoxDependency;
block.add_dependency_id(*new_dep.id());
mg.add_dependency(boxed_dep);
mg.revoke_connection(connection_id, true);
mg.revoke_dependency(connection_id, true);
}
}

Expand Down Expand Up @@ -380,7 +380,7 @@ async fn finish_modules(&self, compilation: &mut Compilation) -> Result<()> {
.expect("should have module");

importer.remove_dependency_id(*connection);
mg.revoke_connection(connection, true);
mg.revoke_dependency(connection, true);
}
}

Expand Down
19 changes: 19 additions & 0 deletions tests/e2e/cases/make/remove-dynamic-entry/index.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import { expect, test } from "@/fixtures";

test("should compile", async ({ page, fileAction, rspack }) => {
await expect(page.getByText("index1")).toBeVisible();
await expect(page.getByText("index2")).toBeVisible();

fileAction.deleteFile("src/index2.js");
fileAction.updateFile("src/index1.js", content => content.replace("div.innerText = \"index1\";", "div.innerText = \"index1 updated\";"));

await expect(async () => {
await page.reload();
expect(await page.locator("#index1").innerText()).toBe("index1 updated");
}).toPass();
await expect(page.locator("#index2")).toHaveCount(0);
await expect(page.locator("#webpack-dev-server-client-overlay")).toHaveCount(0);

const stats = rspack.compiler._lastCompilation?.getStats().toJson({ all: false, errors: true });
expect(stats?.errors?.length).toBe(0);
});
25 changes: 25 additions & 0 deletions tests/e2e/cases/make/remove-dynamic-entry/rspack.config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
const path = require("path")
const fs = require("fs");
const rspack = require("@rspack/core");

/** @type {import("@rspack/core").Configuration} */
module.exports = {
entry: async () => {
const context = path.resolve(__dirname, "src");
const files = await fs.promises.readdir(context);
let entries = files.filter(f => f.startsWith('index'));
entries.sort();
return entries.reduce((acc, e, i) => {
acc[`index${i + 1}`] = path.resolve(context, e);
return acc;
}, {})
},
context: __dirname,
mode: "development",
plugins: [
new rspack.HtmlRspackPlugin(),
],
devServer: {
hot: true
},
};
6 changes: 6 additions & 0 deletions tests/e2e/cases/make/remove-dynamic-entry/src/index1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
Promise.resolve().then(() => {
const div = document.createElement("div");
div.innerText = "index1";
div.id = "index1";
document.body.appendChild(div);
})
4 changes: 4 additions & 0 deletions tests/e2e/cases/make/remove-dynamic-entry/src/index2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
const div = document.createElement("div");
div.innerText = "index2";
div.id = "index2";
document.body.appendChild(div);
Loading