Explore the usage of manganis for assets

[ctron]

Coming from a discussion on #710 this issue tracks the idea of using manganis for assets.

I think there are three levels of integration possible:

1. Through trunk's hook system
2. By having trunk call some external CLI
3. By having trunk use a crate doing the processing

I think we should start with 1, and see how that works.

There was also the idea of trunk having a WebAssembly based extension system. It might be that this could be a use case for this. But this would also mean some bigger effort. Just mentioning this here to not forget about it.

[airblast-dev]

I decided to take a look into this. To add support for manganis, we do not need to call any external commands or use hooks (though there's a few things that could be good to have as configurable in Trunk.toml but thats something we can look into later).

Following the example for cli-support, I was able to get images working. Other assets also work, but will require a bit more done to get things integrated in a clean fashion.

Though, I am not sure on what you meant with the ways we can integrate manganis with trunk. Would you be able to explain what you had in mind?

Apologies if I'm deeply misunderstanding what you meant.

[ctron]

Sounds cool. So here is what I had in mind:

1. Through trunk's hook system:

   We could simply come up with an example, leveraging trunk's hook system to run all required steps. This would give users a blueprint on how to do this today, without any code changes required by trunk (most likely). If we find some gap in the hook system, that would be valuable too. Additionally, we might gather some insight of what would be required for the next steps.

2. By having trunk call some external CLI

   Learning from the hook example, we could automate the process as we do it with other tools today (like wasm-bindgen or sass). If enabled, during the build we call that external tool, and ensure upfront that it is installed (or install it if possible).

3. By having trunk use a crate doing the processing

   If 2.) leaves some usability gaps, which we only know once we've done it, we could explore embedded manganis by adding it as a crate, and directly calling into its API. That would eliminate the step of an external CLI to be called (and installed). But has some downsides too, like the fact that we would include one specific version of manganis.

I still think we should start with 1. … Which would enable users to use it, identify gaps in trunk, and give some insight on how we could improve with 2. and 3..

[airblast-dev]

manganis, and manganis-cli-support are purely libraries to my knowledge, so what command would intended to be called in by hooks or by trunk itself?

For 3, you are right that we can only include a single version. Things will very likely break when a change in https://github.com/DioxusLabs/collect-assets/tree/master/common is introduced.

That being said, at the time of writing manganis does have a few rough edges. The API is a bit limiting, and from what I can tell, will likely require a few workarounds, or changes to manganis to get things integrated in a reasonable way.

I still am not sure how this is to be integrated with 1, and 2. So I think I am going to leave those for someone else to tackle for the time being.

[ctron]

You're right, I missed that point. Judging from manganis-cli-support (naming is hard) I assumed there would be a CLI available.

Which basically rules out 1 and 2 … and I am not a big fan of adding more complexity to the existing rust pipeline of trunk.

Maybe we should take a different route then: create a hook system inside of trunk. Based on a trait. Kind of like a plugin concept, but starting slowly. I guess the current build hooks could be one implementation of this. I also do recall the general idea of having "trunk plugins" (through WASM or maybe even something like deno). But having a test with this with a pure Rust trait might be a good first try.

[airblast-dev]

I agree, though manganis would introduce small changes in the rust pipeline itself. The issue is what surrounds it.

Take tailwind for example. If a user has <link data-trunk rel="tailwind-css" href="..."> in their index.html, and also wants to include other tailwind classes using manganis::classes!("flex flex-col p-5") we would have to merge them, not easy but not too hard. What if the user has multiple tailwind files linked? Which one do we merge the information into?

We could use meta information (MetadataAsset), as a workaround, but I don't like that solution either, as it could result in hard to track bugs when using it. Issue is, trunk wouldn't have a way to know if the meta information is just incorrect, or if it was intended to be used by a hook (where the hook would call an application that also supports manganis). Just to be clear there is ways to get it working, but if we are going to use metadata for a bunch of stuff, and then parse things, we may as well implement our own solution, as by using metadata we would lose some of the practicality that comes by using manganis on the users side.

Point is, there is quite a bit of different cases to consider.

It is also worth noting the manganis library is mainly designed to be used with dioxus, and it does show in its API design. This makes working with it somewhat difficult for our use case. There's also a few small bugs, and a few panics that are easy to trigger. I think if we are going to implement support for manganis we should wait until it has matured a bit more.

I also think the idea of "trunk plugins" will be better for our use case.

[ctron]

Fair points. And that's why the issue has a title of "Explore" not "Implement" 😁 … I think it worth exploring and coming up with either a plan, or a list of point to report back to the folks of manganis.

I also think the idea of "trunk plugins" will be better for our use case.

I think that's a way forward in any case. It's "just" some work. And it's independent of this issue. The idea was to use manganis as a driver for this effort.

[ealmloff]

You're right, I missed that point. Judging from manganis-cli-support (naming is hard) I assumed there would be a CLI available.

manganis-cli-support just contains code that a CLI (like the dioxus CLI or trunk) can use to implement manganis support. Manganis needs to set some settings before a build to communicate with a hook, but all of this could be bundled as an external CLI

I agree, though manganis would introduce small changes in the rust pipeline itself. The issue is what surrounds it.

Take tailwind for example. If a user has in their index.html, and also wants to include other tailwind classes using manganis::classes!("flex flex-col p-5") we would have to merge them, not easy but not too hard. What if the user has multiple tailwind files linked? Which one do we merge the information into?

What about a minimal subset of magnanis that just works with files and doesn't need to effect the HTML directly at all? Trunk could just support mg::file, mg::image, and mg::font that take an asset and output a link to the optimized asset.

It is also worth noting the manganis library is mainly designed to be used with dioxus, and it does show in its API design. This makes working with it somewhat difficult for our use case. There's also a few small bugs, and a few panics that are easy to trigger. I think if we are going to implement support for manganis we should wait until it has matured a bit more.

Fair points. And that's why the issue has a title of "Explore" not "Implement" 😁 … I think > it worth exploring and coming up with either a plan, or a list of point to report back to the folks of manganis.

It would be very useful to have a list of what trunk would need to make it easier to support manganis (A separate CLI? More granular optimization options for each file?). It was developed with dioxus in mind, but I would like to make it easy to use it the broader ecosystem 🙂

[airblast-dev]

What about a minimal subset of magnanis that just works with files and doesn't need to effect the HTML directly at all? Trunk could just support mg::file, mg::image, and mg::font that take an asset and output a link to the optimized asset.

That would be a better way to implement initial support of manganis to trunk. It would likely require relatively little changes on both sides, and it should be usable in a simple fashion.

It would be very useful to have a list of what trunk would need to make it easier to support manganis (A separate CLI? More granular optimization options for each file?). It was developed with dioxus in mind, but I would like to make it easy to use it the broader ecosystem 🙂

I would be happy to provide a list of things that would help 😄 , but first I would like to attempt implementing proper support for a minimal subset of manganis into trunk, taking notes and sharing limitations and what not in the meanwhile. I think that way I can take a better look into things would fit in.

A CLI could be useful in general, in a case a user is trying to hook things up themselves with scripts (in trunk's case hooks), they could still use manganis without requiring support from the tool. In trunk's case, using the manganis-cli-support library would be better when errors occur, as trunk might have more context on why the error occured. Meaning depending on the situation we might be able to give better user feedback. We might be able to do a few other stuff that would make things fit in better with the rest of trunk.

As I also said above, I will start by attempting to implement support for a subset of manganis, and see how things go from there. 👍

[airblast-dev]

Here's a few things I would like to note.

Issues:

• manganis doesn't really work well in workspaces and examples in my experience. At least from what I have seen and done, projects that use trunk, are often in a workspace. I have provided some information in the relevant issue Manganis Fails To Provide Assets In Workspaces & Examples DioxusLabs/manganis#23.

Possible Changes:

Things mentioned here shouldn't be needed for a minimal implementation, but would improve flexibility overall. I'm pretty sure some of the things I mentioned are unreasonable or would be considered nit's, and feel free to ignore them if that is the case.

• Support for generating file hash with SHA256, SHA384 and SHA512. A way for the user to get the hash of the file would be useful in case they want to use Subresource Integrity (or anything else that could benefit from having a file hash). I think this could also be done on trunk's side with a trunk library that extends manganis, but since it might be used in general and not just in trunk, I think it is worth mentioning.
• Return types being anyhow::Result. A concrete error type might be better when implementing CLI support, as the builder might have more context in certain cases. I am not sure if this would be used by trunk, but I think giving the ability to handle certain errors differently could help overall.
• Decreasing number of std::result::Result::unwrap's. Looking at the source code a bit, results are unwrapped somewhat frequently. To be clear the unwrap's are in reasonable places. However it removes the ability to do any cleanup in case a panic occurs (this isn't strictly needed by trunk, however it doesn't mean trunk or any other application will never have to do any cleanup in the future). There's a few ways to avoid this, but I think this could be made a bit more practical on manganis-cli-support's end. This does have the downside of replacing quite a bit with of return type's with std::result::Result. It's worth noting std::panic::catch_unwind could be used, but I think returning a std::result::Result (or anyhow::Result) feels more reasonable in case the application wants to do some cleanup.

There is also this PR being worked on DioxusLabs/manganis#26 which brings quite a bit of changes to how things work, so its possible that some of the things I mentioned will be changed, or fixed once the PR is finalized, and merged.

These are just a few things I thought, that could help in adding support for trunk and possibly other tools.

General Notes for Trunk:

I'm pretty sure that an extending library will be required for good integration with trunk. manganis relies on macro's to avoid keeping any residual information left in the binary. This means to extend manganis we will have to have a similar approach. Technically we could use manganis::meta, and take inputs as a raw string, then serialize using serde. This would work but it can also result in confusing issues for the user as it is very error prone. So I'm more on the side writing our own macro's then internally calling manganis::meta with a deserialized version of the struct. It's completely possible I'm missing an obvious solution here, but this is what I have been able to come up with. This is something that could be done later, after implementing support for a subset of manganis.

[ealmloff]

Thanks for putting a list together!

• Support for generating file hash with SHA256, SHA384 and SHA512...

This would be great to support in Manganis. I opened a tracking issue here: DioxusLabs/manganis#28

• Return types being anyhow::Result...
• Decreasing number of std::result::Result::unwrap's...

There are some places where Manganis currently panics where is should return an error. For file operations or other things that may fail it should return a result. There will likely still be some unwraps for cases that should never fail. Returning a result in those cases cause more work to handle errors that will never occur for CLIs like trunk and the dioxus-cli. For example, I don't think removing an unwrap in this code is helpful "123".try_into::<u32>().unwrap() because it should never fail.

I also agree anyhow makes properly handling errors in CLIs more difficult than it needs to be. I can work on switching to a proper error enum

Tracking issue: DioxusLabs/manganis#27

• manganis doesn't really work well in workspaces and examples in my experience...

There is also this PR being worked on DioxusLabs/collect-assets#26 which brings quite a bit of changes to how things work, so its possible that some of the things I mentioned will be changed, or fixed once the PR is finalized, and merged.

DioxusLabs/collect-assets#26 should fix the workspace issues because it embeds asset links directly into the binary instead of using a global cache. It is mostly an internal change to fix some bugs. Asset types stay the same. I think the only breaking change for trunk is the way loading assets will work. Instead of providing the path to the workspace, trunk will need to load the assets from the path to the WASM