Manganis: Add Flag To Disable Folder Optimization

[DogeDark]

Adds the FolderAssetOptions::new().into_preserved() method for a preserved unoptimized folder. This is planned to be a temporary workaround until we can make breaking changes in 0.7. Below is the original plan for 0.7:

Original Plan:

Adds a optimize_files option for FolderAsset e.g.

const FOLDER_OPTIONS: FolderAssetOptions = FolderAssetOptions::new().with_optimize_files(true);

If the flag is set, the file will go under minor processing. The flag defaults to true to maintain current behavior.

[jkelleyrtp]

I think we originally wanted Folders to retain their asset names internally but have a different name externally - did that change? I'm ambivalent to either so @ealmloff can make the final call

It seems that the extra processing of the folder contents was a mistake / bad default, seems like we want to preserve contents by default and make optimization opt-in

[ealmloff]

What is the advantage of different default optimizations for folders vs files? I would expect these two to point to the same asset:

const FOLDER: ASSET = asset!("/myfolder");
const ASSET: ASSET = asset!("/myfolder/asset.js");

rsx! {
   script { src: "{FOLDER}/asset.js" }
   script { src: ASSET }
}

If asset optimization is breaking some files, we could make it opt in instead of opt out, or just fix the broken optimizations. Either way, I think we should be consistent about the default options regardless of how you import the asset

[DogeDark]

I'm mixed between both approaches as one could say that the FolderAsset is for importing something as-is, but I think file preservation should be opt-in for consistency.

[nicoburns]

IMO asset-optimization ought to be opt-in in general. People wanted asset optimization will likely go looking for a setting and find it easily enough. People not wanting optimization are more likely to be surprised by it happening.

[DogeDark]

What if we:

• Left the default behavior the same (opt out of optimizations)
• Include the with_optimize_files(bool) method.
• We can alter the behavior if desired at a later date (non-patch release).

According to semver, an addition should be on a minor release, not a patch, but I don't believe it would be an issue to include it on a patch.

[jkelleyrtp]

What if we:

• Left the default behavior the same (opt out of optimizations)
• Include the with_optimize_files(bool) method.
• We can alter the behavior if desired at a later date (non-patch release).

According to semver, an addition should be on a minor release, not a patch, but I don't believe it would be an issue to include it on a patch.

I think we should go with this direction. If you are able to make the changes today I can review + merge before EOD, hopefully unblocking the playground deployment!

[jkelleyrtp]

Looks good but as per the comments in the PR we should retain the current default behavior and then decide if we want to change it in the next version.

[jkelleyrtp]

A bit unfortunate issue that the FolderAssetOption struct can't be expanded in a semver compatible way. We'd have to work around it somehow - either with a new feature flag or a new Options type. :(

I added a task to our notion to ensure that all public structs we export are marked non_exhaustive in the future so this doesn't happen again.

Otherwise looks good!

[jkelleyrtp]

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.

[DogeDark]

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.

[jkelleyrtp]

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.

[DogeDark]

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.

[DogeDark]

The hasher test failed and I believe it was because the AssetOptions enum was changed (which is used to calculate the hash). I set the new left to equal the right in the assertion. @ealmloff Can you confirm if this was the case & is fine?

[ealmloff]

The hasher test failed and I believe it was because the AssetOptions enum was changed (which is used to calculate the hash). I set the new left to equal the right in the assertion. @ealmloff Can you confirm if this was the case & is fine?

Yes, the hash is based on the serialized form of the enum. The hash itself changing between versions isn't an issue because it always comes from the macro. However, changing the structure of AssetOptions will break deserialization of the enum with old versions of the CLI.

If you create a new project that points to this version of dioxus and try to serve it with the old version of the CLI, no assets are copied

[DogeDark]

I don't see a way around breaking enum deserialization since we need to pass an identifier to preserve files somehow.