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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support passing custom profiles to cargo #295

Closed
wants to merge 2 commits into from
Closed

Support passing custom profiles to cargo #295

wants to merge 2 commits into from

Conversation

dnaka91
Copy link
Contributor

@dnaka91 dnaka91 commented Jan 12, 2022

Checklist

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 馃.

Reasoning

Rust recently (since 1.56) added support for custom profiles. This is especially convenient in a workspace where for example frontend and backend live together in the same repo as 2 crates of a workspace.

If a crate is part of a workspace it can't define its own profile settings, thus there's only a single global release profile. But often you want different profile settings for server and frontend, the server optimized for speed, frontend for binary size. With this new feature, you can define a custom profile optimized for binary size and use it for the frontend only, while setting up the release profile for performance for every other crate.

Description

This PR adds support for the cargo flag to Trunk, so you can configure which profile to use. Sadly, we have a few automatic settings that depend on the release flag and there is no proper solution when used with a profile.

These spots are:

  • Decide whether to run wasm-opt.
  • Decide to build CSS as compressed or expanded.

Currently, this is solved by treating a custom profile the same way as the release flag, but I'm sure that isn't what everyone wants. For example, if someone creates a custom profile for debug purposes, they surely don't want wasm-opt to be run.

Improvements

To solve this issue, it would be best to change Trunk to follow the profile style. Something like:

Trunk.toml

# default debug profile
[profile.debug]
wasm-opt = 0
sass-style = "expanded"

# default release profile
[profile.release]
wasm-opt = 3
sass-style = "compressed"

# custom `minified` profile
[profile.minified]
wasm-opt = "z"
sass-style = "compressed"

These would be in sync with cargo's profiles, thus activating as follows:

  • trunk build or trunk build --profile debug -> debug
  • trunk build --release or trunk build --profile release -> release
  • trunk build --profile minified -> minified

Also, for wasm-opt we can set the optimization level in the HTML file with <link data-trunk rel="rust" data-wasm-opt="z">. We might either remove it or keep it and decide what takes precedence. I would personally go with removing it to make the configuration easier and avoid mistakes and surprises. If we decide to keep it, it probably makes sense to choose the HTML setting over Trunk.toml?

Further details

I didn't update the changelog and other docs yet, as I feel we have to discuss a bit further about the profiles, and just wanted to get a bit of input on this idea. I believe adding profiles to Trunk directly is very beneficial but would break current setups. Therefore, I didn't add it yet until we make further decisions.

@dnaka91 dnaka91 requested a review from thedodd January 12, 2022 02:12
@dnaka91 dnaka91 self-assigned this Jan 12, 2022
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Looking really solid, but I'd say we should add some docs about this to the site. That will also help me to understand the full breadth of this feature. Thanks boss!

@dnaka91
Copy link
Contributor Author

dnaka91 commented Jun 30, 2022

Actually, I started on this, then realized that it complicates the whole situation with different profiles for different assets. So better to not merge this until we eventually move to something better for the profile management overall.

For example, as we discussed in the past, to get all the special HTML tags out, and do all configuration in the Trunk.toml, so we can have Trunk profiles that are in sync with Cargo profiles and allow to specifically set overrides for different assets.

@finnbear
Copy link

finnbear commented Aug 10, 2022

For now, a workaround is to edit the normal release profile from client/.cargo/config.toml, which shouldn't affect other members.

AFAIK the same profile configuration is possible as in Cargo.toml

Edit: This workaround is problematic since it might force rust to recompile the packages too often.

@dnaka91 dnaka91 closed this by deleting the head repository Sep 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants