Skip to content

Commit

Permalink
fix: file dependencies are lost when css module fails (#8328)
Browse files Browse the repository at this point in the history
  • Loading branch information
LingyuCoder authored Nov 5, 2024
1 parent 0763068 commit 886a427
Show file tree
Hide file tree
Showing 28 changed files with 431 additions and 175 deletions.
1 change: 1 addition & 0 deletions crates/node_binding/binding.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -659,6 +659,7 @@ export interface JsLoaderContext {
loaderItems: Array<JsLoaderItem>
loaderIndex: number
loaderState: Readonly<JsLoaderState>
__internal__error?: JsRspackError
}

export interface JsLoaderItem {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::collections::HashMap;

use napi::bindgen_prelude::*;
use napi_derive::napi;
use rspack_binding_values::{JsModuleWrapper, JsResourceData};
use rspack_binding_values::{JsModuleWrapper, JsResourceData, JsRspackError};
use rspack_core::{LoaderContext, RunnerContext};
use rspack_error::error;
use rspack_loader_runner::{LoaderItem, State as LoaderState};
Expand Down Expand Up @@ -81,6 +81,8 @@ pub struct JsLoaderContext {
pub loader_index: i32,
#[napi(ts_type = "Readonly<JsLoaderState>")]
pub loader_state: JsLoaderState,
#[napi(js_name = "__internal__error")]
pub error: Option<JsRspackError>,
}

impl TryFrom<&mut LoaderContext<RunnerContext>> for JsLoaderContext {
Expand Down Expand Up @@ -137,6 +139,7 @@ impl TryFrom<&mut LoaderContext<RunnerContext>> for JsLoaderContext {
loader_items: cx.loader_items.iter().map(Into::into).collect(),
loader_index: cx.loader_index,
loader_state: cx.state().into(),
error: None,
})
}
}
19 changes: 17 additions & 2 deletions crates/rspack_binding_options/src/plugins/js_loader/scheduler.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use napi::Either;
use rspack_core::{
AdditionalData, LoaderContext, NormalModuleLoaderShouldYield, NormalModuleLoaderStartYielding,
RunnerContext, BUILTIN_LOADER_PREFIX,
diagnostics::CapturedLoaderError, AdditionalData, LoaderContext, NormalModuleLoaderShouldYield,
NormalModuleLoaderStartYielding, RunnerContext, BUILTIN_LOADER_PREFIX,
};
use rspack_error::{error, Result};
use rspack_hook::plugin_hook;
Expand Down Expand Up @@ -46,6 +46,21 @@ pub(crate) fn merge_loader_context(
to: &mut LoaderContext<RunnerContext>,
mut from: JsLoaderContext,
) -> Result<()> {
if let Some(error) = from.error {
return Err(
CapturedLoaderError::new(
error.message,
error.stack,
error.hide_stack,
from.file_dependencies,
from.context_dependencies,
from.missing_dependencies,
from.build_dependencies,
)
.into(),
);
}

to.cacheable = from.cacheable;
to.file_dependencies = from.file_dependencies.into_iter().map(Into::into).collect();
to.context_dependencies = from
Expand Down
59 changes: 27 additions & 32 deletions crates/rspack_binding_values/src/compilation/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -633,7 +633,7 @@ impl JsCompilation {
.module_executor
.as_ref()
.expect("should have module executor");
let result = module_executor
let res = module_executor
.import_module(
request,
layer,
Expand All @@ -643,37 +643,32 @@ impl JsCompilation {
original_module.map(ModuleIdentifier::from),
)
.await;
match result {
Ok(res) => {
let js_result = JsExecuteModuleResult {
cacheable: res.cacheable,
file_dependencies: res
.file_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
context_dependencies: res
.context_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
build_dependencies: res
.build_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
missing_dependencies: res
.missing_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
assets: res.assets.into_iter().collect(),
id: res.id,
};
Ok(js_result)
}
Err(e) => Err(Error::new(napi::Status::GenericFailure, format!("{e}"))),
}
let js_result = JsExecuteModuleResult {
cacheable: res.cacheable,
file_dependencies: res
.file_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
context_dependencies: res
.context_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
build_dependencies: res
.build_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
missing_dependencies: res
.missing_dependencies
.into_iter()
.map(|d| d.to_string_lossy().to_string())
.collect(),
assets: res.assets.into_iter().collect(),
id: res.id,
};
Ok(js_result)
})
}

Expand Down
79 changes: 39 additions & 40 deletions crates/rspack_core/src/compiler/module_executor/execute.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@ use std::{iter::once, sync::atomic::AtomicU32};

use itertools::Itertools;
use rspack_collections::{Identifier, IdentifierSet};
use rspack_error::Result;
use rustc_hash::FxHashMap as HashMap;
use rustc_hash::FxHashSet as HashSet;
use tokio::sync::oneshot::Sender;
Expand Down Expand Up @@ -31,6 +30,7 @@ pub type ExecuteModuleId = u32;

#[derive(Debug, Default)]
pub struct ExecuteModuleResult {
pub error: Option<String>,
pub cacheable: bool,
pub file_dependencies: HashSet<PathBuf>,
pub context_dependencies: HashSet<PathBuf>,
Expand All @@ -48,7 +48,7 @@ pub struct ExecuteTask {
pub public_path: Option<PublicPath>,
pub base_uri: Option<String>,
pub result_sender: Sender<(
Result<ExecuteModuleResult>,
ExecuteModuleResult,
CompilationAssets,
IdentifierSet,
Vec<ExecutedRuntimeModule>,
Expand Down Expand Up @@ -217,39 +217,37 @@ impl Task<MakeTaskContext> for ExecuteTask {
);

let module_graph = compilation.get_module_graph();
let mut execute_result = match exports {
let mut execute_result = modules.iter().fold(
ExecuteModuleResult {
cacheable: true,
id,
..Default::default()
},
|mut res, m| {
let module = module_graph.module_by_identifier(m).expect("unreachable");
let build_info = &module.build_info();
if let Some(info) = build_info {
res
.file_dependencies
.extend(info.file_dependencies.iter().cloned());
res
.context_dependencies
.extend(info.context_dependencies.iter().cloned());
res
.missing_dependencies
.extend(info.missing_dependencies.iter().cloned());
res
.build_dependencies
.extend(info.build_dependencies.iter().cloned());
if !info.cacheable {
res.cacheable = false;
}
}
res
},
);
match exports {
Ok(_) => {
let mut result = modules.iter().fold(
ExecuteModuleResult {
cacheable: true,
..Default::default()
},
|mut res, m| {
let module = module_graph.module_by_identifier(m).expect("unreachable");
let build_info = &module.build_info();
if let Some(info) = build_info {
res
.file_dependencies
.extend(info.file_dependencies.iter().cloned());
res
.context_dependencies
.extend(info.context_dependencies.iter().cloned());
res
.missing_dependencies
.extend(info.missing_dependencies.iter().cloned());
res
.build_dependencies
.extend(info.build_dependencies.iter().cloned());
if !info.cacheable {
res.cacheable = false;
}
}
res
},
);

result.id = id;

for m in modules.iter() {
let codegen_result = codegen_results.get(m, Some(&runtime));

Expand All @@ -264,18 +262,19 @@ impl Task<MakeTaskContext> for ExecuteTask {
);
}
}

Ok(result)
}
Err(e) => Err(e),
Err(e) => {
execute_result.cacheable = false;
execute_result.error = Some(e.to_string());
}
};

let assets = std::mem::take(compilation.assets_mut());
let code_generated_modules = std::mem::take(&mut compilation.code_generated_modules);
let module_assets = std::mem::take(&mut compilation.module_assets);
if let Ok(ref mut result) = execute_result {
result.assets = assets.keys().cloned().collect::<HashSet<_>>();
result.assets.extend(
if execute_result.error.is_none() {
execute_result.assets = assets.keys().cloned().collect::<HashSet<_>>();
execute_result.assets.extend(
module_assets
.values()
.flat_map(|m| m.iter().map(|i| i.to_owned()).collect_vec())
Expand Down
5 changes: 2 additions & 3 deletions crates/rspack_core/src/compiler/module_executor/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ use dashmap::{mapref::entry::Entry, DashSet};
pub use execute::ExecuteModuleId;
pub use execute::ExecutedRuntimeModule;
use rspack_collections::{Identifier, IdentifierDashMap, IdentifierDashSet};
use rspack_error::Result;
use tokio::sync::{
mpsc::{unbounded_channel, UnboundedSender},
oneshot,
Expand Down Expand Up @@ -189,7 +188,7 @@ impl ModuleExecutor {
base_uri: Option<String>,
original_module_context: Option<Context>,
original_module_identifier: Option<Identifier>,
) -> Result<ExecuteModuleResult> {
) -> ExecuteModuleResult {
let sender = self
.event_sender
.as_ref()
Expand Down Expand Up @@ -226,7 +225,7 @@ impl ModuleExecutor {
let (execute_result, assets, code_generated_modules, executed_runtime_modules) =
rx.await.expect("should receiver success");

if let Ok(execute_result) = &execute_result
if execute_result.error.is_none()
&& let Some(original_module_identifier) = original_module_identifier
{
self
Expand Down
92 changes: 90 additions & 2 deletions crates/rspack_core/src/diagnostics.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,14 @@
use std::fmt::Display;
use std::{fmt::Display, path::PathBuf};

use itertools::Itertools;
use rspack_error::{
impl_diagnostic_transparent,
error, impl_diagnostic_transparent,
miette::{self, Diagnostic},
thiserror::{self, Error},
DiagnosticExt, Error, TraceableError,
};
use rspack_util::ext::AsAny;
use rustc_hash::FxHashSet;

use crate::{BoxLoader, DependencyRange};

Expand Down Expand Up @@ -147,6 +148,93 @@ impl ModuleParseError {
}
}

#[derive(Debug)]
pub struct CapturedLoaderError {
pub message: String,
pub stack: Option<String>,
pub hide_stack: Option<bool>,
pub file_dependencies: Vec<String>,
pub context_dependencies: Vec<String>,
pub missing_dependencies: Vec<String>,
pub build_dependencies: Vec<String>,
}

impl CapturedLoaderError {
pub fn new(
message: String,
stack: Option<String>,
hide_stack: Option<bool>,
file_dependencies: Vec<String>,
context_dependencies: Vec<String>,
missing_dependencies: Vec<String>,
build_dependencies: Vec<String>,
) -> Self {
Self {
message,
stack,
hide_stack,
file_dependencies,
context_dependencies,
missing_dependencies,
build_dependencies,
}
}

pub fn take_message(&mut self) -> String {
std::mem::take(&mut self.message)
}

pub fn take_stack(&mut self) -> Option<String> {
std::mem::take(&mut self.stack)
}

pub fn take_file_dependencies(&mut self) -> FxHashSet<PathBuf> {
std::mem::take(&mut self.file_dependencies)
.into_iter()
.map(Into::into)
.collect()
}

pub fn take_context_dependencies(&mut self) -> FxHashSet<PathBuf> {
std::mem::take(&mut self.context_dependencies)
.into_iter()
.map(Into::into)
.collect()
}

pub fn take_missing_dependencies(&mut self) -> FxHashSet<PathBuf> {
std::mem::take(&mut self.missing_dependencies)
.into_iter()
.map(Into::into)
.collect()
}

pub fn take_build_dependencies(&mut self) -> FxHashSet<PathBuf> {
std::mem::take(&mut self.build_dependencies)
.into_iter()
.map(Into::into)
.collect()
}
}

impl std::error::Error for CapturedLoaderError {
fn source(&self) -> ::core::option::Option<&(dyn std::error::Error + 'static)> {
None
}
}

impl std::fmt::Display for CapturedLoaderError {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
write!(f, "CapturedLoaderError: {}", self.message)
}
}

impl rspack_error::miette::Diagnostic for CapturedLoaderError {
fn code<'a>(&'a self) -> Option<Box<dyn std::fmt::Display + 'a>> {
Some(Box::new("CapturedLoaderError"))
}
}

/// Mark boxed errors as [crate::diagnostics::ModuleParseError],
/// then, map it to diagnostics
pub fn map_box_diagnostics_to_module_parse_diagnostics(
Expand Down
Loading

2 comments on commit 886a427

@rspack-bot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Benchmark detail: Open

Name Base (2024-11-05 9c56263) Current Change
10000_big_production-mode + exec 43.9 s ± 638 ms 43.7 s ± 979 ms -0.38 %
10000_development-mode + exec 1.83 s ± 22 ms 1.83 s ± 21 ms -0.02 %
10000_development-mode_hmr + exec 644 ms ± 4.4 ms 645 ms ± 11 ms +0.10 %
10000_production-mode + exec 2.41 s ± 19 ms 2.43 s ± 23 ms +0.98 %
arco-pro_development-mode + exec 1.75 s ± 55 ms 1.79 s ± 70 ms +2.50 %
arco-pro_development-mode_hmr + exec 430 ms ± 1.5 ms 430 ms ± 1.4 ms +0.05 %
arco-pro_production-mode + exec 3.21 s ± 75 ms 3.21 s ± 66 ms -0.17 %
arco-pro_production-mode_generate-package-json-webpack-plugin + exec 3.24 s ± 57 ms 3.24 s ± 51 ms +0.20 %
threejs_development-mode_10x + exec 1.58 s ± 7.7 ms 1.59 s ± 8.8 ms +0.60 %
threejs_development-mode_10x_hmr + exec 771 ms ± 19 ms 776 ms ± 10 ms +0.65 %
threejs_production-mode_10x + exec 4.95 s ± 27 ms 4.98 s ± 27 ms +0.65 %

@rspack-bot
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Ran ecosystem CI: Open

suite result
modernjs ❌ failure
_selftest ✅ success
rspress ✅ success
rslib ✅ success
rsbuild ❌ failure
examples ❌ failure
devserver ✅ success

Please sign in to comment.