feat(turbopack): support webpack-like output.css_filename & output.as…#116
feat(turbopack): support webpack-like output.css_filename & output.as…#116
Conversation
…set_module_filename
Summary of ChangesHello @xusd320, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances Turbopack's browser chunking context by introducing a more granular and configurable system for generating output filenames. It allows developers to define custom templates for initial CSS, non-initial CSS, and general asset modules, providing greater control over the output structure and aligning with more advanced bundling requirements. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
xusd320
changed the title
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces filename templating for CSS and asset modules, which is a great enhancement for customization. However, I've found a critical issue in the asset module filename generation that could lead to incorrect filenames and data loss (the content hash). Additionally, there are opportunities to improve the efficiency of type checking for CSS chunks and to complete the implementation for distinguishing between initial and non-initial CSS chunk filenames. My review includes suggestions to address these points.
| let asset_path = match &self.asset_module_filename { | ||
| Some(filename_template) => { | ||
| let mut filename = filename_template.to_string(); | ||
|
|
||
| if match_name_placeholder(&filename) { | ||
| filename = replace_name_placeholder(&filename, basename); | ||
| } | ||
|
|
||
| if match_content_hash_placeholder(&filename) { | ||
| filename = replace_content_hash_placeholder(&filename, &content_hash); | ||
| }; | ||
|
|
||
| if let Some(ext) = source_path.extension_ref() { | ||
| if let Some((stem, _)) = filename.rsplit_once(".") { | ||
| filename = stem.to_string(); | ||
| } | ||
| filename = format!("{filename}.{ext}"); | ||
| } | ||
|
|
||
| filename | ||
| } | ||
| None => match source_path.extension_ref() { | ||
| Some(ext) => format!( | ||
| "{basename}.{content_hash}.{ext}", | ||
| basename = &basename[..basename.len() - ext.len() - 1], | ||
| content_hash = &content_hash[..8] | ||
| ), | ||
| None => format!( | ||
| "{basename}.{content_hash}", | ||
| content_hash = &content_hash[..8] | ||
| ), | ||
| }, |
There was a problem hiding this comment.
The current implementation for asset module filename templating has a critical flaw that can lead to incorrect filenames by removing the content hash.
- Incorrect
[name]replacement: Thebasenamepassed toreplace_name_placeholderincludes the file extension. The[name]placeholder should represent the filename without the extension. - Buggy extension handling: The logic at lines 782-787 unconditionally strips everything after the last dot. If the
[contenthash]is placed after a dot in the template (e.g.,[name].[contenthash]), the hash will be removed from the filename.
This suggested change fixes these issues by correctly extracting the filename stem for the [name] placeholder and removing the flawed extension-replacement logic. The template is now expected to define the full filename. A more robust long-term solution would be to support an [ext] placeholder.
let asset_path = match &self.asset_module_filename {
Some(filename_template) => {
let name_part = source_path.file_stem().unwrap_or(basename);
let mut filename = filename_template.to_string();
if match_name_placeholder(&filename) {
filename = replace_name_placeholder(&filename, name_part);
}
if match_content_hash_placeholder(&filename) {
filename = replace_content_hash_placeholder(&filename, &content_hash);
}
filename
}
None => match source_path.extension_ref() {
Some(ext) => format!(
"{basename}.{content_hash}.{ext}",
basename = &basename[..basename.len() - ext.len() - 1],
content_hash = &content_hash[..8]
),
None => format!(
"{basename}.{content_hash}",
content_hash = &content_hash[..8]
),
},| let resolved_asset = asset.to_resolved().await?; | ||
| if ResolvedVc::try_downcast_type::<CssChunk>(resolved_asset).is_some() | ||
| || ResolvedVc::try_downcast_type::<CssChunkSourceMapAsset>(resolved_asset) | ||
| .is_some() | ||
| { | ||
| // TODO: distinguash initial or non-initial css chunk, the non-initial css | ||
| // chunk should use css_chunk_filename for template | ||
| this.css_filename.clone() | ||
| } else { | ||
| this.chunk_filename.clone() | ||
| } |
There was a problem hiding this comment.
This type check is inefficient because it requires resolving the Vc to a concrete asset via to_resolved().await? before checking its type. This can be optimized by using Vc::is::<T>(), which performs the type check without needing to resolve the full asset content, leading to better performance.
Consider refactoring to something like:
let is_css = asset.is::<CssChunk>().await?
|| asset.is::<CssChunkSourceMapAsset>().await?;
if is_css {
// ...
} else {
// ...
}This would avoid the await on to_resolved() just for a type check.
| // TODO: distinguash initial or non-initial css chunk, the non-initial css | ||
| // chunk should use css_chunk_filename for template | ||
| this.css_filename.clone() |
There was a problem hiding this comment.
The implementation currently uses css_filename for all CSS chunks. However, the css_chunk_filename option was also introduced for non-initial CSS chunks but is not being used. The TODO comment acknowledges this. To complete this feature, logic to distinguish between initial and non-initial CSS chunks should be added to make use of css_chunk_filename as intended.
2 participants
This pull request enhances the
BrowserChunkingContextin theturbopack-browsercrate to provide more flexible and customizable filename templating for CSS and asset modules. The changes introduce new configuration options for specifying filename templates for initial and non-initial CSS chunks, as well as asset modules, and update logic to use these templates when generating output filenames.The most important changes are:
Filename templating for CSS and asset modules:
BrowserChunkingContextforcss_filename,css_chunk_filename, andasset_module_filenameto allow custom filename templates for initial CSS chunks, non-initial CSS chunks, and asset modules, respectively.Builder API enhancements:
css_filename,css_chunk_filename,asset_module_filename) toBrowserChunkingContextBuilderfor setting the new filename template fields.Dependency updates:
turbopack-cssas a dependency to enable CSS chunk detection and handling.turbopack_cssfor CSS chunk handling.Initialization and defaults:
Defaultimplementation forBrowserChunkingContextto initialize the new filename template fields.