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

Restore manganis optimizations #3195

Merged
merged 46 commits into from
Nov 26, 2024
Merged

Conversation

ealmloff
Copy link
Member

@ealmloff ealmloff commented Nov 11, 2024

This PR will restore the bulk of manganis with a slightly different serialization approach. It also restores all of the documentation that was removed in #2779. Instead of parsing the builder inside of the macro and serializing that into json, this PR serializes the builder directly. This should make it easier to add new asset types to manganis and make the macro significantly more flexible.

Previously the macro would only accept literals, now it will be able to accept any const type:

const ASSET: Asset = asset!("/assets/image.png");
const OPTIONS: ImageAssetOptions = ImageAssetOptions::new().with_format(ImageFormat::Avif);
const AVIF_ASSET: Asset = asset!("/assets/image.png", OPTIONS);

The path itself still needs to be a literal string so we can validate and hash the contents at compile time, but the rest of the settings are just const rust

closes #3229

@ealmloff ealmloff added the manganis Related to the manganis crate label Nov 11, 2024
@ealmloff ealmloff marked this pull request as ready for review November 18, 2024 17:22
@ealmloff
Copy link
Member Author

I tried testing this on Windows 10 and I got this behavior:

  • trying to use a folder asset and it errors with access denied
  • assets are optimized and the optimized assets are placed in my top-level assets folder (nothing about assets in the target folder)
  • the link returned by manganis for use in img src/document::Link is weird/doesn't work:
http://127.0.0.1:8080/assets///?\C:\Users\Darka\Documents\Projects\DioxusLabs\temp\web-manganis-test-3195\assets\main-e6ddccc1e64cc12d.css=

All of those issues should be fixed now (tested on windows 11). There was a path separator issue in manganis on windows

@jkelleyrtp jkelleyrtp self-requested a review November 20, 2024 13:46
Copy link
Member

@jkelleyrtp jkelleyrtp left a comment

Choose a reason for hiding this comment

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

Small tweaks:

  • might be good to pull out the assets module from the CLI into its own crate
  • some of the code we generate in the macro could be moved into a const function or two
  • some docs in some places refernce the old mg!() or are incomplete, presumably since they're already incomplete.
  • it would be nice if the serialized bytes were valid json but I get how the const_serialize can't do that in a general case.
  • double check any issues with us not properly cleaning the build dirs - I want to make sure there's no weirdness with leftover html/assets/gradle files still showing up between builds leaving sticky state behind.

Overall much better!

packages/const-serialize/Cargo.toml Outdated Show resolved Hide resolved
packages/manganis-core/src/asset.rs Outdated Show resolved Hide resolved
packages/manganis-core/src/css.rs Outdated Show resolved Hide resolved
packages/manganis-core/src/images.rs Outdated Show resolved Hide resolved
///
/// ```rust
/// # use manganis::{asset, Asset, ImageAssetOptions};
/// const _: Asset = asset!("/assets/image.png", ImageAssetOptions::new().with_preload(true));
Copy link
Member

Choose a reason for hiding this comment

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

ImageAsset::options() might be more discoverable?

Copy link
Member Author

@ealmloff ealmloff Nov 20, 2024

Choose a reason for hiding this comment

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

ImageAsset doesn't exist. We can't support metadata about assets like lqip or the sha without injecting it in the linker, so I didn't add it in this PR because of the time constraint

Comment on lines 28 to 33
// Hash the contents along with the asset config to create a unique hash for the asset
// When this hash changes, the client needs to re-fetch the asset
let mut hasher = ConstHasher::new();
hasher = hasher.write(&content_hash.to_le_bytes());
hasher = hasher.hash_by_bytes(asset_config);
let hash = hasher.finish();
Copy link
Member

Choose a reason for hiding this comment

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

do we want to be actually hashing assets at compile time? I'm a bit worried about long compile times - maybe there's a way we can inject metadata into the assets on the filesystem such that the hash gets included into the name during macro expansion?

Copy link
Member Author

Choose a reason for hiding this comment

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

Metadata isn't preserved in git, so we shouldn't use it for asset versioning. We want the hash to change only when the asset actually changes so the client can use the cached version if possible. When we were using the hash based on the metadata last modified time, it changed every time you pulled the git repo which causes a lot of issues in CI.

We could only hash the file in release mode if compile times are an issue. In debug mode, we disable caching anyway so it doesn't matter.

Copy link
Member Author

@ealmloff ealmloff Nov 20, 2024

Choose a reason for hiding this comment

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

The hashing here should be really cheap, hashing the contents in the file in the macro could get expensive. I'm not sure how we can check if a macro is being expanded from a debug or release mode crate

packages/manganis-core/Cargo.toml Outdated Show resolved Hide resolved
packages/const-serialize-macro/Cargo.toml Outdated Show resolved Hide resolved
Comment on lines +108 to +143
swc = "=0.283.0"
swc_allocator = { version = "=0.1.8", default-features = false }
swc_atoms = { version = "=0.6.7", default-features = false }
swc_cached = { version = "=0.3.20", default-features = false }
swc_common = { version = "=0.37.5", default-features = false }
swc_compiler_base = { version = "=0.19.0", default-features = false }
swc_config = { version = "=0.1.15", default-features = false }
swc_config_macro = { version = "=0.1.4", default-features = false }
swc_ecma_ast = { version = "=0.118.2", default-features = false }
swc_ecma_codegen = { version = "=0.155.1", default-features = false }
swc_ecma_codegen_macros = { version = "=0.7.7", default-features = false }
swc_ecma_compat_bugfixes = { version = "=0.12.0", default-features = false }
swc_ecma_compat_common = { version = "=0.11.0", default-features = false }
swc_ecma_compat_es2015 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2016 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2017 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2018 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2019 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2020 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2021 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es2022 = { version = "=0.12.0", default-features = false }
swc_ecma_compat_es3 = { version = "=0.12.0", default-features = false }
swc_ecma_ext_transforms = { version = "=0.120.0", default-features = false }
swc_ecma_lints = { version = "=0.100.0", default-features = false }
swc_ecma_loader = { version = "=0.49.1", default-features = false }
swc_ecma_minifier = { version = "=0.204.0", default-features = false }
swc_ecma_parser = { version = "=0.149.1", default-features = false }
swc_ecma_preset_env = { version = "=0.217.0", default-features = false, features = [
"serde",
] }
swc_ecma_transforms = { version = "=0.239.0", default-features = false }
swc_ecma_transforms_base = { version = "=0.145.0", default-features = false }
swc_ecma_transforms_classes = { version = "=0.134.0", default-features = false }
swc_ecma_transforms_compat = { version = "=0.171.0", default-features = false }
swc_ecma_transforms_macros = { version = "=0.5.5", default-features = false }
swc_ecma_transforms_module = { version = "=0.190.0", default-features = false }
Copy link
Member

Choose a reason for hiding this comment

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

it really would not be terrible to cut out the asset system (Manifest, optimizer, etc) into its own crate again... and maybe with some feature flags so we can gate dev mode dx to not have to include all this.

Copy link
Member Author

Choose a reason for hiding this comment

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

Agreed. I would like to pull out asset optimization and resolution into a separate crate, but I don't want to block the release and it would take longer to make an more stable API. How about a follow up PR after more pressing issues like the base path are fixed? I have an issue ready once this is merged

packages/cli/src/build/bundle.rs Show resolved Hide resolved
@ealmloff
Copy link
Member Author

it would be nice if the serialized bytes were valid json but I get how the const_serialize can't do that in a general case.

I think that is possible, but it would require a lot more logic to parse and format strings and numbers at compile time. I'm not sure how that would effect compile times, but it could be worth exploring in the future

I don't love having a bespoke serialization format. https://github.com/jamesmunns/postcard is a much simpler well defined serialization format that might be easier to target than json. I think it is pretty similar to what we are currently generating

@DogeDark
Copy link
Member

All of those issues should be fixed now (tested on windows 11). There was a path separator issue in manganis on windows

I still have all the same issues on the latest commit except the folder asset now panics as todo during the CLI's asset stage

@ealmloff
Copy link
Member Author

  • assets are optimized and the optimized assets are placed in my top-level assets folder (nothing about assets in the target folder)
  • the link returned by manganis for use in img src/document::Link is weird/doesn't work:

Can you share the code you used that causes those issues with the latest version? This code works as I would expect on windows:

use dioxus::prelude::*;

fn main() {
    launch(|| {
        let asset = asset!("/assets/img.png");
        rsx! {
            img {
                src: "{asset}"
            }
        }
    })
}

@DogeDark
Copy link
Member

@ealmloff https://github.com/DogeDark/manganis-3195

@ealmloff
Copy link
Member Author

@ealmloff https://github.com/DogeDark/manganis-3195

Thanks, that example is working now as well. I was testing windows on desktop where path separator in the CLI is the same as the target platform path separator. WASM has unix path separators so it was stripping the wrong path when cross building to web

@DogeDark
Copy link
Member

Thanks, that example is working now as well.

I can confirm, it works!

@jkelleyrtp jkelleyrtp merged commit f9617c8 into DioxusLabs:main Nov 26, 2024
17 checks passed
@ealmloff ealmloff deleted the restore-manganis branch November 26, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
manganis Related to the manganis crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Manganis 0.6.0-alpha.5 using image() in asset! results in macro expansion ignores token , and any following
3 participants