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: loader intermediate data should be overridden between loaders #7846

Merged
merged 2 commits into from
Sep 10, 2024
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
3 changes: 2 additions & 1 deletion .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -42,5 +42,6 @@
"tokio",
"ukey",
"Ukey"
]
],
"rust-analyzer.checkOnSave": false
}
Original file line number Diff line number Diff line change
Expand Up @@ -31,36 +31,43 @@ impl Debug for CssExtractRspackAdditionalDataPlugin {
}

#[plugin_hook(NormalModuleAdditionalData for CssExtractRspackAdditionalDataPlugin)]
async fn additional_data(&self, additional_data: &mut AdditionalData) -> Result<()> {
if !additional_data.contains::<ThreadsafeJsValueRef<Unknown>>() {
async fn additional_data(&self, additional_data: &mut Option<&mut AdditionalData>) -> Result<()> {
if !additional_data
.as_ref()
.is_some_and(|data| data.contains::<ThreadsafeJsValueRef<Unknown>>())
{
return Ok(());
}
let (tx, rx) = oneshot::channel::<AdditionalData>();
let mut old_data = std::mem::take(additional_data);
self.js_callback.call(Box::new(move |env| {
if let Some(data) = old_data.get::<ThreadsafeJsValueRef<Unknown>>()
&& let Ok(data) = data.get(env)
&& let Ok(data) = data.coerce_to_object()
&& let Ok(Some(data)) = data.get::<_, String>("css-extract-rspack-plugin")
{
let data_list: Vec<rspack_plugin_extract_css::CssExtractJsonData> = data
.split("__RSPACK_CSS_EXTRACT_SEP__")
.map(|info| {
serde_json::from_str(info)
.unwrap_or_else(|e| panic!("failed to parse CssExtractJsonData: {}", e))
})
.collect();
if let Some(mut old_data) = additional_data.as_mut().map(|data| std::mem::take(*data)) {
let (tx, rx) = oneshot::channel::<AdditionalData>();
self.js_callback.call(Box::new(move |env| {
if let Some(data) = old_data
.get::<ThreadsafeJsValueRef<Unknown>>()
.and_then(|data| data.get(env).ok())
.and_then(|data| data.coerce_to_object().ok())
.and_then(|data| data.get::<_, String>("css-extract-rspack-plugin").ok())
.flatten()
{
let data_list: Vec<rspack_plugin_extract_css::CssExtractJsonData> = data
.split("__RSPACK_CSS_EXTRACT_SEP__")
.map(|info| {
serde_json::from_str(info)
.unwrap_or_else(|e| panic!("failed to parse CssExtractJsonData: {}", e))
})
.collect();

old_data.insert(CssExtractJsonDataList(data_list));
};
tx.send(old_data)
.expect("should send `additional_data` for `CssExtractRspackAdditionalDataPlugin`");
}));
let new_data = rx
.await
.expect("should receive `additional_data` for `CssExtractRspackAdditionalDataPlugin`");
// ignore the default value here
let _ = std::mem::replace(additional_data, new_data);
old_data.insert(CssExtractJsonDataList(data_list));
};
tx.send(old_data)
.expect("should send `additional_data` for `CssExtractRspackAdditionalDataPlugin`");
}));
let new_data = rx
.await
.expect("should receive `additional_data` for `CssExtractRspackAdditionalDataPlugin`");
if let Some(data) = additional_data.as_mut() {
let _ = std::mem::replace(*data, new_data);
}
}
Ok(())
}

Expand Down
10 changes: 5 additions & 5 deletions crates/rspack_binding_options/src/plugins/js_loader/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -94,17 +94,17 @@ impl TryFrom<&mut LoaderContext<RunnerContext>> for JsLoaderContext {
.to_js_module()
.expect("CompilerModuleContext::to_js_module should not fail."),
hot: cx.hot,
content: match &cx.content {
content: match cx.content() {
Some(c) => Either::B(c.to_owned().into_bytes().into()),
None => Either::A(Null),
},
additional_data: cx
.additional_data
.get::<ThreadsafeJsValueRef<Unknown>>()
.additional_data()
.and_then(|data| data.get::<ThreadsafeJsValueRef<Unknown>>())
.cloned(),
source_map: cx
.source_map
.clone()
.source_map()
.cloned()
.map(|v| v.to_json())
.transpose()
.map_err(|e| error!(e.to_string()))?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ pub fn get_builtin_loader(builtin: &str, options: Option<&str>) -> Result<BoxLoa
rspack_loader_preact_refresh::PreactRefreshLoader::default().with_identifier(builtin.into()),
));
}

// TODO: should be compiled with a different cfg
if builtin.starts_with(rspack_loader_testing::SIMPLE_ASYNC_LOADER_IDENTIFIER) {
return Ok(Arc::new(rspack_loader_testing::SimpleAsyncLoader));
}
Expand All @@ -72,6 +74,12 @@ pub fn get_builtin_loader(builtin: &str, options: Option<&str>) -> Result<BoxLoa
if builtin.starts_with(rspack_loader_testing::PITCHING_LOADER_IDENTIFIER) {
return Ok(Arc::new(rspack_loader_testing::PitchingLoader));
}
if builtin.starts_with(rspack_loader_testing::PASS_THROUGH_LOADER_IDENTIFIER) {
return Ok(Arc::new(rspack_loader_testing::PassthroughLoader));
}
if builtin.starts_with(rspack_loader_testing::NO_PASS_THROUGH_LOADER_IDENTIFIER) {
return Ok(Arc::new(rspack_loader_testing::NoPassthroughLoader));
}
unreachable!("Unexpected builtin loader: {builtin}")
}

Expand Down
20 changes: 13 additions & 7 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::{
LoaderContext, NormalModuleLoaderShouldYield, NormalModuleLoaderStartYielding, RunnerContext,
BUILTIN_LOADER_PREFIX,
AdditionalData, LoaderContext, NormalModuleLoaderShouldYield, NormalModuleLoaderStartYielding,
RunnerContext, BUILTIN_LOADER_PREFIX,
};
use rspack_error::{error, Result};
use rspack_hook::plugin_hook;
Expand Down Expand Up @@ -46,9 +46,6 @@ pub(crate) fn merge_loader_context(
to: &mut LoaderContext<RunnerContext>,
mut from: JsLoaderContext,
) -> Result<()> {
if let Some(data) = &from.additional_data {
to.additional_data.insert(data.clone());
}
to.cacheable = from.cacheable;
to.file_dependencies = from.file_dependencies.into_iter().map(Into::into).collect();
to.context_dependencies = from
Expand All @@ -66,16 +63,23 @@ pub(crate) fn merge_loader_context(
.into_iter()
.map(Into::into)
.collect();
to.content = match from.content {

let content = match from.content {
Either::A(_) => None,
Either::B(c) => Some(rspack_core::Content::from(Into::<Vec<u8>>::into(c))),
};
to.source_map = from
let source_map = from
.source_map
.as_ref()
.map(|s| rspack_core::rspack_sources::SourceMap::from_slice(s))
.transpose()
.map_err(|e| error!(e.to_string()))?;
let additional_data = from.additional_data.take().map(|data| {
let mut additional = AdditionalData::default();
additional.insert(data);
additional
});
to.__finish_with((content, source_map, additional_data));

// update loader status
to.loader_items = to
Expand All @@ -90,6 +94,8 @@ pub(crate) fn merge_loader_context(
to.set_pitch_executed()
}
to.set_data(from.data);
// JS loader should always be considered as finished
to.set_finish_called();
to
})
.collect();
Expand Down
7 changes: 2 additions & 5 deletions crates/rspack_core/src/normal_module.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ define_hook!(NormalModuleLoader: SyncSeries(loader_context: &mut LoaderContext<R
define_hook!(NormalModuleLoaderShouldYield: SyncSeriesBail(loader_context: &LoaderContext<RunnerContext>) -> bool);
define_hook!(NormalModuleLoaderStartYielding: AsyncSeries(loader_context: &mut LoaderContext<RunnerContext>));
define_hook!(NormalModuleBeforeLoaders: SyncSeries(module: &mut NormalModule));
define_hook!(NormalModuleAdditionalData: AsyncSeries(additional_data: &mut AdditionalData));
define_hook!(NormalModuleAdditionalData: AsyncSeries(additional_data: &mut Option<&mut AdditionalData>));

#[derive(Debug, Default)]
pub struct NormalModuleHooks {
Expand Down Expand Up @@ -419,14 +419,11 @@ impl Module for NormalModule {
current_loader: Default::default(),
});

let additional_data = AdditionalData::default();

let loader_result = run_loaders(
self.loaders.clone(),
self.resource_data.clone(),
Some(plugin.clone()),
build_context.runner_context,
additional_data,
)
.await;
let (mut loader_result, ds) = match loader_result {
Expand Down Expand Up @@ -457,7 +454,7 @@ impl Module for NormalModule {
.plugin_driver
.normal_module_hooks
.additional_data
.call(&mut loader_result.additional_data)
.call(&mut loader_result.additional_data.as_mut())
.await?;
self.add_diagnostics(ds);

Expand Down
2 changes: 1 addition & 1 deletion crates/rspack_core/src/parser_and_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ pub struct ParseContext<'a> {
pub loaders: &'a [BoxLoader],
pub resource_data: &'a ResourceData,
pub compiler_options: &'a CompilerOptions,
pub additional_data: AdditionalData,
pub additional_data: Option<AdditionalData>,
pub build_info: &'a mut BuildInfo,
pub build_meta: &'a mut BuildMeta,
}
Expand Down
15 changes: 8 additions & 7 deletions crates/rspack_loader_lightningcss/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ impl LightningCssLoader {

let filename = resource_path.as_str().to_string();

let Some(content) = std::mem::take(&mut loader_context.content) else {
let Some(content) = loader_context.take_content() else {
return Ok(());
};

Expand Down Expand Up @@ -131,8 +131,7 @@ impl LightningCssLoader {
let enable_sourcemap = loader_context.context.module_source_map_kind.enabled();

let mut source_map = loader_context
.source_map
.as_ref()
.source_map()
.map(|input_source_map| -> Result<_> {
let mut sm = parcel_sourcemap::SourceMap::new(
input_source_map
Expand Down Expand Up @@ -180,15 +179,17 @@ impl LightningCssLoader {
})
.map_err(|_| rspack_error::error!("failed to generate css"))?;

loader_context.content = Some(rspack_core::Content::String(content.code));

if enable_sourcemap {
let source_map = source_map
.to_json(None)
.map_err(|e| rspack_error::error!(e.to_string()))?;

loader_context.source_map =
Some(SourceMap::from_json(&source_map).expect("should be able to generate source-map"));
loader_context.finish_with((
content.code,
SourceMap::from_json(&source_map).expect("should be able to generate source-map"),
));
} else {
loader_context.finish_with(content.code);
}

Ok(())
Expand Down
6 changes: 4 additions & 2 deletions crates/rspack_loader_preact_refresh/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ impl PreactRefreshLoader {
#[async_trait::async_trait]
impl Loader<RunnerContext> for PreactRefreshLoader {
async fn run(&self, loader_context: &mut LoaderContext<RunnerContext>) -> Result<()> {
let content = std::mem::take(&mut loader_context.content).expect("Content should be available");
let content = loader_context
.take_content()
.expect("Content should be available");
let mut source = content.try_into_string()?;
source += "\n";
source += include_str!("runtime.js");
loader_context.content = Some(source.into());
loader_context.finish_with(source);
Ok(())
}
}
Expand Down
4 changes: 2 additions & 2 deletions crates/rspack_loader_react_refresh/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ impl ReactRefreshLoader {
#[async_trait::async_trait]
impl Loader<RunnerContext> for ReactRefreshLoader {
async fn run(&self, loader_context: &mut LoaderContext<RunnerContext>) -> Result<()> {
let Some(content) = std::mem::take(&mut loader_context.content) else {
let Some(content) = loader_context.take_content() else {
return Ok(());
};
let mut source = content.try_into_string()?;
Expand All @@ -42,7 +42,7 @@ Promise.resolve().then(function() {
$ReactRefreshRuntime$.refresh(__webpack_module__.id, __webpack_module__.hot);
});
"#;
loader_context.content = Some(source.into());
loader_context.finish_with(source);
Ok(())
}
}
Expand Down
Loading
Loading