Skip to content

Commit

Permalink
turbopack-css: fix rule duplication (vercel/turborepo#6778)
Browse files Browse the repository at this point in the history
This fixes an issue in css chunk generation where rules could be
duplicated, which was likely introduced in the chunking refactor (a
series of PRs starting with vercel/turborepo#6104).

CSS chunk generation had special logic that would walk the module graph
on its own and include code. Combined with the topological list of
modules provided by chunking, this unnecessarily included duplicate
rules in incorrect order.

Test Plan: Added and updated snapshot tests.


Closes PACK-2141
  • Loading branch information
wbinnssmith committed Dec 14, 2023
1 parent cbc494b commit 5c31bf2
Show file tree
Hide file tree
Showing 40 changed files with 394 additions and 359 deletions.
81 changes: 80 additions & 1 deletion crates/turbopack-core/src/reference_type.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
use std::fmt::Display;

use anyhow::Result;
use indexmap::IndexMap;
use turbo_tasks::Vc;

Expand Down Expand Up @@ -43,10 +44,84 @@ pub enum EcmaScriptModulesReferenceSubType {
Undefined,
}

/// The individual set of conditions present on this module through `@import`
#[derive(Debug)]
#[turbo_tasks::value(shared)]
pub struct ImportAttributes {
pub layer: Option<String>,
pub supports: Option<String>,
pub media: Option<String>,
}

/// The accumulated list of conditions that should be applied to this module
/// through its import path
#[derive(Debug, Default)]
#[turbo_tasks::value]
pub struct ImportContext {
pub layers: Vec<String>,
pub supports: Vec<String>,
pub media: Vec<String>,
}

#[turbo_tasks::value_impl]
impl ImportContext {
#[turbo_tasks::function]
pub fn new(layers: Vec<String>, media: Vec<String>, supports: Vec<String>) -> Vc<Self> {
ImportContext {
layers,
media,
supports,
}
.cell()
}

#[turbo_tasks::function]
pub async fn add_attributes(
self: Vc<Self>,
attr_layer: Option<String>,
attr_media: Option<String>,
attr_supports: Option<String>,
) -> Result<Vc<Self>> {
let this = &*self.await?;

let layers = {
let mut layers = this.layers.clone();
if let Some(attr_layer) = attr_layer {
if !layers.contains(&attr_layer) {
layers.push(attr_layer.to_owned());
}
}
layers
};

let media = {
let mut media = this.media.clone();
if let Some(attr_media) = attr_media {
if !media.contains(&attr_media) {
media.push(attr_media.to_owned());
}
}
media
};

let supports = {
let mut supports = this.supports.clone();
if let Some(attr_supports) = attr_supports {
if !supports.contains(&attr_supports) {
supports.push(attr_supports.to_owned());
}
}
supports
};

Ok(ImportContext::new(layers, media, supports))
}
}

#[turbo_tasks::value(serialization = "auto_for_input")]
#[derive(Debug, Clone, PartialOrd, Ord, Hash)]
pub enum CssReferenceSubType {
AtImport,
AtImport(Option<Vc<ImportContext>>),
Compose,
/// Reference from any asset to a CSS-parseable asset.
///
Expand Down Expand Up @@ -143,6 +218,10 @@ impl ReferenceType {
matches!(other, ReferenceType::EcmaScriptModules(_))
&& matches!(sub_type, EcmaScriptModulesReferenceSubType::Undefined)
}
ReferenceType::Css(CssReferenceSubType::AtImport(_)) => {
// For condition matching, treat any AtImport pair as identical.
matches!(other, ReferenceType::Css(CssReferenceSubType::AtImport(_)))
}
ReferenceType::Css(sub_type) => {
matches!(other, ReferenceType::Css(_))
&& matches!(sub_type, CssReferenceSubType::Undefined)
Expand Down
8 changes: 8 additions & 0 deletions crates/turbopack-css/src/asset.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use turbopack_core::{
ident::AssetIdent,
module::Module,
reference::{ModuleReference, ModuleReferences},
reference_type::ImportContext,
resolve::origin::ResolveOrigin,
source::Source,
};
Expand All @@ -33,6 +34,7 @@ fn modifier() -> Vc<String> {
pub struct CssModuleAsset {
source: Vc<Box<dyn Source>>,
asset_context: Vc<Box<dyn AssetContext>>,
import_context: Option<Vc<ImportContext>>,
ty: CssModuleAssetType,
use_lightningcss: bool,
}
Expand All @@ -46,10 +48,12 @@ impl CssModuleAsset {
asset_context: Vc<Box<dyn AssetContext>>,
ty: CssModuleAssetType,
use_lightningcss: bool,
import_context: Option<Vc<ImportContext>>,
) -> Vc<Self> {
Self::cell(CssModuleAsset {
source,
asset_context,
import_context,
ty,
use_lightningcss,
})
Expand All @@ -71,6 +75,8 @@ impl ParseCss for CssModuleAsset {
Ok(parse_css(
this.source,
Vc::upcast(self),
this.import_context
.unwrap_or_else(|| ImportContext::new(vec![], vec![], vec![])),
this.ty,
this.use_lightningcss,
))
Expand Down Expand Up @@ -273,6 +279,7 @@ impl CssChunkItem for CssModuleChunkItem {
Ok(CssChunkItemContent {
inner_code: output_code.to_owned().into(),
imports,
import_context: self.module.await?.import_context,
source_map: Some(*source_map),
}
.into())
Expand All @@ -284,6 +291,7 @@ impl CssChunkItem for CssModuleChunkItem {
)
.into(),
imports: vec![],
import_context: None,
source_map: None,
}
.into())
Expand Down
60 changes: 48 additions & 12 deletions crates/turbopack-css/src/chunk/mod.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
pub(crate) mod single_item_chunk;
pub mod source_map;
pub(crate) mod writer;

use std::fmt::Write;

Expand All @@ -23,10 +22,10 @@ use turbopack_core::{
},
module::Module,
output::{OutputAsset, OutputAssets},
reference_type::ImportContext,
server_fs::ServerFileSystem,
source_map::{GenerateSourceMap, OptionSourceMap},
};
use writer::expand_imports;

use self::{single_item_chunk::chunk::SingleItemCssChunk, source_map::CssChunkSourceMapAsset};
use crate::{process::ParseCssResultSourceMap, util::stringify_js, ImportAssetReference};
Expand Down Expand Up @@ -65,21 +64,56 @@ impl CssChunk {

let this = self.await?;

let mut code = CodeBuilder::default();
let mut body = CodeBuilder::default();
let mut external_imports = IndexSet::new();
for css_item in this.content.await?.chunk_items.iter() {
// TODO(WEB-1261)
for external_import in expand_imports(&mut body, *css_item).await? {
external_imports.insert(external_import.await?.to_owned());
for css_item in &this.content.await?.chunk_items {
let id = &*css_item.id().await?;

let content = &css_item.content().await?;
for import in &content.imports {
if let CssImport::External(external_import) = import {
external_imports.insert((*external_import.await?).to_string());
}
}

writeln!(body, "/* {} */", id)?;
let mut close: Vec<String> = vec![];
if let Some(import_context) = content.import_context {
let import_context = &*import_context.await?;
if !&import_context.layers.is_empty() {
writeln!(body, "@layer {} {{", import_context.layers.join("."))?;
close.push("}\n".to_owned());
}
if !&import_context.media.is_empty() {
writeln!(body, "@media {} {{", import_context.media.join(" and "))?;
close.push("}\n".to_owned());
}
if !&import_context.supports.is_empty() {
writeln!(
body,
"@supports {} {{",
import_context.supports.join(" and ")
)?;
close.push("}\n".to_owned());
}
}

body.push_source(&content.inner_code, content.source_map.map(Vc::upcast));
writeln!(body)?;

for line in &close {
body.push_source(&Rope::from(line.to_string()), None);
}
writeln!(body)?;
}

let mut code = CodeBuilder::default();
for external_import in external_imports {
writeln!(code, "@import {};", stringify_js(&external_import))?;
}

code.push_code(&body.build());
let built = &body.build();
code.push_code(built);

if *this
.chunking_context
Expand All @@ -88,9 +122,9 @@ impl CssChunk {
&& code.has_source_map()
{
let chunk_path = self.path().await?;
write!(
writeln!(
code,
"\n/*# sourceMappingURL={}.map*/",
"/*# sourceMappingURL={}.map*/",
chunk_path.file_name()
)?;
}
Expand Down Expand Up @@ -316,18 +350,20 @@ pub trait CssChunkPlaceable: ChunkableModule + Module + Asset {}
#[turbo_tasks::value(transparent)]
pub struct CssChunkPlaceables(Vec<Vc<Box<dyn CssChunkPlaceable>>>);

#[derive(Clone)]
#[derive(Clone, Debug)]
#[turbo_tasks::value(shared)]
pub enum CssImport {
External(Vc<String>),
Internal(Vc<ImportAssetReference>, Vc<Box<dyn CssChunkItem>>),
Composes(Vc<Box<dyn CssChunkItem>>),
}

#[derive(Debug)]
#[turbo_tasks::value(shared)]
pub struct CssChunkItemContent {
pub inner_code: Rope,
pub import_context: Option<Vc<ImportContext>>,
pub imports: Vec<CssImport>,
pub inner_code: Rope,
pub source_map: Option<Vc<ParseCssResultSourceMap>>,
}

Expand Down
87 changes: 0 additions & 87 deletions crates/turbopack-css/src/chunk/writer.rs

This file was deleted.

Loading

0 comments on commit 5c31bf2

Please sign in to comment.