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

Require opt-in to enable optimizations #175

Closed
wants to merge 1 commit into from
Closed

Require opt-in to enable optimizations #175

wants to merge 1 commit into from

Conversation

udoprog
Copy link

@udoprog udoprog commented May 4, 2021

Checklist

  • [*] Updated CHANGELOG.md describing pertinent changes.
  • [*] Updated README.md 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 :).

This requires the user to explicitly enable optimizations through --optimize or by using the optimize option in Trunk.toml for potentially expensive optimizations to run.

This option is sorely needed, because optimizations might be enabled through static configuration in a <link data-wasm-opt="z"> statement in index.html, but you don't necessarily want that to run while working locally. So without another toggle you constantly have to edit your index.html file instead.

This is also a breaking change.

It might also be desirable to implicitly enable optimizations if --release is specified, but in that case I'd like to have a --no-optimize option as well, because I sometimes want to enable release builds while developing locally without optimizing as well. I'd prefer to add this separate to this PR though, because I haven't fully wrapped my head around the config module yet and how to best make multiple options interoperate.

@thedodd
Copy link
Member

thedodd commented May 17, 2021

@udoprog optimizations are actually disabled by default (as of now), and even after #146 lands, optimizations can be disabled by setting data-wasm-opt="0" (https://trunkrs.dev/assets/#rust).

Long-term, I would say what we actually want to do is stand on the shoulders of giants and follow this pattern: https://rustwasm.github.io/wasm-pack/book/cargo-toml-configuration.html (profile based controls over optimizations and levels).

I would say that we should close this, as it is not needed.

@thedodd thedodd added the disposition:close The tagged item can probably be closed label May 17, 2021
@udoprog
Copy link
Author

udoprog commented May 17, 2021

@udoprog optimizations are actually disabled by default (as of now), and even after #146 lands, optimizations can be disabled by setting data-wasm-opt="0" (https://trunkrs.dev/assets/#rust).

This is the editing being referred to. I've had instances where I've accidentally left data-wasm-opt="0" in the release build.

I'm glad you have a long term idea. But please consider accepting a shorter term fix initially that can be nuked once the long term is in place. This would significantly improve my experience working with trunk.

@thedodd
Copy link
Member

thedodd commented Aug 5, 2021

@udoprog sorry about the very late response on my part. Your points here are definitely valid, and I like the path that you've mentioned about associating wasm-opt with --release profile, however providing a --no-opt option to disable wasm-opt even for --release mode would just be the same as disabling wasm-opt entirely, no?

Anyway, take a look at the discussion over here: #197

@thedodd thedodd removed the disposition:close The tagged item can probably be closed label Aug 6, 2021
thedodd added a commit that referenced this pull request Aug 6, 2021
This addresses a few pain points where there was no way to reasonably
disable wasm-opt use for debug builds while keeping it enabled for
release builds. There seems to be very few (if any) use cases for using
wasm-opt on a debug build, as such, wasm-opt is now only run on release
builds when enabled.

closes #197
closes #175
thedodd added a commit that referenced this pull request Aug 6, 2021
This addresses a few pain points where there was no way to reasonably
disable wasm-opt use for debug builds while keeping it enabled for
release builds. There seems to be very few (if any) use cases for using
wasm-opt on a debug build, as such, wasm-opt is now only run on release
builds when enabled.

closes #197
closes #175
@thedodd thedodd closed this in #217 Aug 6, 2021
thedodd added a commit that referenced this pull request Aug 6, 2021
This addresses a few pain points where there was no way to reasonably
disable wasm-opt use for debug builds while keeping it enabled for
release builds. There seems to be very few (if any) use cases for using
wasm-opt on a debug build, as such, wasm-opt is now only run on release
builds when enabled.

closes #197
closes #175
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.

2 participants