-
-
Notifications
You must be signed in to change notification settings - Fork 833
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
Conversation
All of those issues should be fixed now (tested on windows 11). There was a path separator issue in manganis on windows |
There was a problem hiding this 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/manganis-core/src/images.rs
Outdated
/// | ||
/// ```rust | ||
/// # use manganis::{asset, Asset, ImageAssetOptions}; | ||
/// const _: Asset = asset!("/assets/image.png", ImageAssetOptions::new().with_preload(true)); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
// 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(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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 |
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 |
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}"
}
}
})
} |
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 |
I can confirm, it works! |
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:
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