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

Minify HTML & CSS, optimize PNGs #449

Closed
wants to merge 4 commits into from
Closed

Conversation

Carlrs
Copy link
Contributor

@Carlrs Carlrs commented Oct 25, 2022

Based on issue #7

Saves a few kilobytes per fresh uncached page load, will be more significant if/when we can get JS minification working (I'm hoping to get one of the potential JS minification libraries for Rust working properly, rather than using yet another commandline tool, though I won't rule that out either).

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 🤓.

@simbleau
Copy link
Member

Really happy to see a PR for this. Thanks! Will review this soon.

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.

Thanks for tackling this! Great work so far, just a few small items.

src/pipelines/mod.rs Outdated Show resolved Hide resolved
src/pipelines/html.rs Outdated Show resolved Hide resolved
@ranile
Copy link
Contributor

ranile commented Oct 26, 2022

This should also minify the JS files created by wasm-bindgen. One approach to do that is by using SWC

@thedodd
Copy link
Member

thedodd commented Oct 27, 2022

@hamza1311 yea, I am tempted to say that we might want to tread carefully before we pull in SWC. I think it will be awesome to do so, however at that point, we will have everything needed to do full JS/TS transpilation. We have to be fairly measured about whether we really want to push Trunk into that space.

I am fairly biased towards allowing folks to use hooks or something similar for integration with other tools. As soon as we cross that threshold, we take on the full yoke of the JS ecosystem.

@dnaka91
Copy link
Contributor

dnaka91 commented Oct 27, 2022

@hamza1311 @thedodd Despite its name, the minify-html crate already includes a JS minifier. Why don't we use that on the wasm-bindgen output?

We definitely should draw a line somewhere though, as thedodd mentioned. For example, for projects that build the whole application in Rust, it's great to have trunk minify everything, as you don't have to tap into any extra tools from the JS ecosystem. But for projects that only use wasm partially or mix and match with JS, it's in my opinion best to leave further bundling to another tool.

@Carlrs
Copy link
Contributor Author

Carlrs commented Oct 27, 2022 via email

@ranile
Copy link
Contributor

ranile commented Oct 27, 2022

Oh, that's too bad. Can you link to the issue you created here?

@dnaka91
Copy link
Contributor

dnaka91 commented Oct 27, 2022

This one, I believe: wilsonzlin/ecma-rs#3

The minify-js crate is relatively new, so maybe best to wait a bit more, before we use it, or we might print a warning and continue on, in case it fails to minify the content. Before they use esbuild which pulls in a whole Go project, so I'm really happy they started building a pure Rust implementation instead. Just needs a bit more battle-testing to become more stable over various inputs.

@Carlrs
Copy link
Contributor Author

Carlrs commented Oct 28, 2022

Author of minify-html/minify-js added a fix for parsing import.meta so I'll see about adding JS minification to this PR later today.

I'm thinking we could minify wasm-bindgen output by default and maybe make minifying other scripts optional either through an extra command-line argument to Trunk, or by checking some additional attribute on the script elements so people can choose case-by-case when to use JS minification? Latter option would be great if people run into issues with minification on one of their scripts but not others, I reckon.

@thedodd
Copy link
Member

thedodd commented Mar 22, 2023

Just checking in to see if this PR is again ready for review? Maybe a rebase is a good idea too.

@Carlrs
Copy link
Contributor Author

Carlrs commented Mar 24, 2023

At one point I was having issues with it, some with the crates being used (and some of it user error on my behalf), so I decided to let it rest a bit and let the minify-js crate mature a bit, as it was in fairly active development.

I think now is a good time to take a new look at it and test it with new and improved dependencies. I'll try to get around to it in the next few days.

@xfbs
Copy link
Contributor

xfbs commented Mar 26, 2023

@Carlrs I would like to see this feature merged as well. If there is anything I can do to help, feel free to ping me. Would love to help any way I can.

@Carlrs
Copy link
Contributor Author

Carlrs commented Apr 5, 2023

Rebased and fixed the issues I'd had previously (using minify-js in wrong TopLevelMode + not checking for image type for before running oxipng). Let me know how it looks!

@Carlrs Carlrs requested a review from thedodd April 15, 2023 09:45
@MolotovCherry
Copy link

MolotovCherry commented Jun 10, 2023

Do you plan to keep the closing tags, as well as minify js/css in the html? (These flags are disabled by default)

let mut minify_cfg = Cfg::spec_compliant();
minify_cfg.minify_css = true;
minify_cfg.minify_js = true;
minify_cfg.keep_closing_tags = true;

In any case, thanks for this PR. I got the trunk executable I wanted now because of it (along with my own fixed edits above)

.await
.with_context(|| format!("error reading file for copying {:?}", &self.path))?;

bytes = if minify {
match file_type {
AssetFileType::Css => minify_html::minify(&bytes, &Cfg::spec_compliant()),
Copy link

@MolotovCherry MolotovCherry Jun 10, 2023

Choose a reason for hiding this comment

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

Here, Cfg::spec_compliant() only initializes basic flags, but leaves the rest to default, so the minify_css field ends up as false.

Was this intended?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, it was not intentional

@Carlrs
Copy link
Contributor Author

Carlrs commented Jun 11, 2023

Added your suggested fixes and bumped the minify-html crate version just in case.

@thedodd could we get a review here sometime?

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Oct 10, 2023
@ctron
Copy link
Collaborator

ctron commented Oct 19, 2023

I cherry-picked this into trunk-ng. I will test it for a bit and when there are no problems, it should be released with 0.17.11.

@github-actions github-actions bot removed the Stale label Oct 20, 2023
@simbleau simbleau mentioned this pull request Oct 21, 2023
@ctron
Copy link
Collaborator

ctron commented Oct 24, 2023

This is now released with trunk-ng 0.17.11

Copy link

github-actions bot commented Dec 9, 2023

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Dec 9, 2023
@ctron
Copy link
Collaborator

ctron commented Dec 12, 2023

This was brought back to trunk with PR #623 and it should be part of the next release of trunk too.

@ctron
Copy link
Collaborator

ctron commented Dec 13, 2023

This is part of the trunk 0.18.0 release

@ctron ctron closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants