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

Manganis: Add Flag To Disable Folder Optimization #3293

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from 4 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
78 changes: 52 additions & 26 deletions packages/cli-opt/src/file.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
use anyhow::Context;
use manganis_core::{AssetOptions, CssAssetOptions, ImageAssetOptions, JsAssetOptions};
use manganis_core::{
AssetOptions, CssAssetOptions, FolderAssetOptions, ImageAssetOptions, JsAssetOptions,
};
use std::path::Path;

use crate::css::process_scss;
Expand All @@ -15,17 +17,10 @@ pub fn process_file_to(
source: &Path,
output_path: &Path,
) -> anyhow::Result<()> {
// If the file already exists, then we must have a file with the same hash
// already. The hash has the file contents and options, so if we find a file
// with the same hash, we probably already created this file in the past
if output_path.exists() {
if check_output_path(output_path)? {
return Ok(());
}
if let Some(parent) = output_path.parent() {
if !parent.exists() {
std::fs::create_dir_all(parent)?;
}
}

match options {
AssetOptions::Unknown => match source.extension().map(|e| e.to_string_lossy()).as_deref() {
Some("css") => {
Expand All @@ -44,20 +39,10 @@ pub fn process_file_to(
process_image(&ImageAssetOptions::new(), source, output_path)?;
}
Some(_) | None => {
if source.is_dir() {
process_folder(source, output_path)?;
} else {
let source_file = std::fs::File::open(source)?;
let mut reader = std::io::BufReader::new(source_file);
let output_file = std::fs::File::create(output_path)?;
let mut writer = std::io::BufWriter::new(output_file);
std::io::copy(&mut reader, &mut writer).with_context(|| {
format!(
"Failed to write file to output location: {}",
output_path.display()
)
})?;
}
match source.is_dir() {
true => process_folder(&FolderAssetOptions::new(), source, output_path)?,
false => copy_file_to(source, output_path)?,
};
}
},
AssetOptions::Css(options) => {
Expand All @@ -69,8 +54,8 @@ pub fn process_file_to(
AssetOptions::Image(options) => {
process_image(options, source, output_path)?;
}
AssetOptions::Folder(_) => {
process_folder(source, output_path)?;
AssetOptions::Folder(options) => {
process_folder(options, source, output_path)?;
}
_ => {
tracing::warn!("Unknown asset options: {:?}", options);
Expand All @@ -79,3 +64,44 @@ pub fn process_file_to(

Ok(())
}

/// Copies an asset to it's destination without any processing.
pub fn copy_file_to(source: &Path, output_path: &Path) -> anyhow::Result<()> {
if check_output_path(output_path)? {
return Ok(());
}

let source_file = std::fs::File::open(source)?;
let mut reader = std::io::BufReader::new(source_file);
let output_file = std::fs::File::create(output_path)?;
let mut writer = std::io::BufWriter::new(output_file);
std::io::copy(&mut reader, &mut writer).with_context(|| {
format!(
"Failed to write file to output location: {}",
output_path.display()
)
})?;

Ok(())
}

/// Check the asset output path to ensure that:
/// 1. The asset doesn't already exist.
/// 2. That the parent path exists or is created.
///
/// Returns true if asset processing should be skipped.
fn check_output_path(output_path: &Path) -> anyhow::Result<bool> {
DogeDark marked this conversation as resolved.
Show resolved Hide resolved
// If the file already exists, then we must have a file with the same hash
// already. The hash has the file contents and options, so if we find a file
// with the same hash, we probably already created this file in the past
if output_path.exists() {
return Ok(true);
}
if let Some(parent) = output_path.parent() {
if !parent.exists() {
std::fs::create_dir_all(parent)?;
}
}

Ok(false)
}
21 changes: 14 additions & 7 deletions packages/cli-opt/src/folder.rs
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
use std::path::Path;

use rayon::iter::{IntoParallelRefIterator, ParallelIterator};

use super::file::process_file_to;
use crate::file::copy_file_to;
use manganis_core::FolderAssetOptions;
use rayon::iter::{IntoParallelRefIterator, ParallelIterator};
use std::path::Path;

/// Process a folder, optimizing and copying all assets into the output folder
pub fn process_folder(source: &Path, output_folder: &Path) -> anyhow::Result<()> {
pub fn process_folder(
options: &FolderAssetOptions,
source: &Path,
output_folder: &Path,
) -> anyhow::Result<()> {
// Create the folder
std::fs::create_dir_all(output_folder)?;

Expand All @@ -21,9 +25,12 @@ pub fn process_folder(source: &Path, output_folder: &Path) -> anyhow::Result<()>
let metadata = file.metadata()?;
let output_path = output_folder.join(file.strip_prefix(source)?);
if metadata.is_dir() {
process_folder(&file, &output_path)
process_folder(options, &file, &output_path)
} else {
process_file_minimal(&file, &output_path)
match options.optimize_files() {
true => process_file_minimal(&file, &output_path),
false => copy_file_to(&file, &output_path),
}
}
})?;

Expand Down
22 changes: 20 additions & 2 deletions packages/manganis/manganis-core/src/folder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ use crate::AssetOptions;
serde::Serialize,
serde::Deserialize,
)]
pub struct FolderAssetOptions {}
pub struct FolderAssetOptions {
/// If the folder's files should be optimized.
optimize_files: bool,
Copy link
Member

Choose a reason for hiding this comment

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

Very unfortunate, but this struct was public and had no fields, meaning before this change it could be crafted and now it can't, meaning this technically doesn't follow semver.

We should have added a #[non_exhaustive] attribute on this strict.

To make this semver-safe we can add a new type that is non_exhaustive and use that as the FolderOptions going forward, marking the old one as deprecated. IE in the AssetOptions enum, we add a new variant that is something like "FolderLiteral" or something.

https://doc.rust-lang.org/cargo/reference/semver.html#struct-add-public-field-when-no-private

Take a look at our options if you can - we might be do something like converting FolderAssetOptions to PreservedFolder or something with a utility method.

Copy link
Member Author

@DogeDark DogeDark Jan 8, 2025

Choose a reason for hiding this comment

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

I want to avoid extra types and such and if this is only a temporary solution until 0.7, I'd be fine with it.

Originally the playground didn't need this PR but at one point, optimizations were enabled for the original asset/public folder breaking the workaround.

Copy link
Member

Choose a reason for hiding this comment

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

Making it temporary is fine or forcing an opt-in with a feature flag might also work.
That way in the playground you can directly import manganis-core in the dep tree and enable the feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have gone with a new asset option type PreservedFolderAssetOptions. The FolderAssetOptions type has an into_preserved() method which converts it into the preserved type.

Additionally, a trait FolderAsset has been created so that cli-opt can accept either asset options for a folder asset. All the temporary structs, methods, and traits are marked doc_hidden to discourage use.

Let me know what you think.

}

impl Default for FolderAssetOptions {
fn default() -> Self {
Expand All @@ -25,7 +28,22 @@ impl Default for FolderAssetOptions {
impl FolderAssetOptions {
/// Create a new folder asset using the builder
pub const fn new() -> Self {
Self {}
Self {
optimize_files: false,
}
}

/// Set whether the folder's files should be optimized.
#[allow(unused)]
pub const fn with_optimize_files(self, preserve_files: bool) -> Self {
Self {
optimize_files: preserve_files,
}
}

/// Check if the folder's files should be optimized.
pub const fn optimize_files(&self) -> bool {
self.optimize_files
}

/// Convert the options into options for a generic asset
Expand Down
Loading