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.

Sign up for GitHub

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

Jump to bottom

fix(bundling): correctly generate minified css bundle #1595

Merged
merged 2 commits into from
Jan 4, 2024
Merged

fix(bundling): correctly generate minified css bundle #1595

merged 2 commits into from
Jan 4, 2024

Conversation

giamir
Copy link
Contributor

@giamir giamir commented Jan 3, 2024 •
edited
Loading

STACKS-366

Context

While merging dependabot PRs I noticed that we got a lot of warning when running npm run build.

[w:bundle]   (875:3) from "postcss-calc" plugin: Parse error on line 1:
[w:bundle]   ...r(--theme-topbar-height, calc(var(--su-static48) + var(--su-static8))) * var(--...
[w:bundle]   ------------------------------------------------------------------------^
[w:bundle]   Expecting end of input, "ADD", "SUB", "MUL", "DIV", got unexpected "RPAREN"
[w:bundle]
[w:bundle]   Code:
[w:bundle]     transform:translate3d(0,calc(var(--theme-topbar-height, calc(var(--su-static48) + var(--su-static8))) * var(--_no-x-offset)),0)

[w:bundle]   WARNING in ./lib/stacks.less (./lib/stacks.less.webpack[javascript/auto]!=!./node_modules/css-loader/dist/cjs.js??ruleSet[1].rules[1].use[1]!./node_modules/postcss-loader/dist/cjs.js??ruleSet[1].rules[1].use[2]!./node_modules/less-loader/dist/cjs.js!./lib/stacks.less)
[w:bundle]   Module Warning (from ./node_modules/postcss-loader/dist/cjs.js):
[w:bundle]   (1817:5) from "postcss-svgo" plugin: SvgoParserError: <input>:1:1: Non-whitespace before first tag.
[w:bundle]
[w:bundle]   > 1 | %3Csvg width='11' height='11' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M1...
[w:bundle]       | ^
[w:bundle]
[w:bundle]
[w:bundle]   Code:
[w:bundle]     --_ch-bg-image:url("data:image/svg+xml,%3Csvg width='11' height='11' xmlns='http://www.w3.org/2000/svg'%3E%3Cpath d='M10 3.41L8.59 2 4 6.59 2.41 5 1 6.41l3 3z' fill='hsl(210, 3.0000000000000027%, 15%)'/%3E%3C/svg%3E")

The command, between other things, creates a minified version of our css bundle using cssnano.

cssnano runs its default presets that includes postcss-svgo and postcss-calc. Those plugins were responsible for the warnings and the incorrect minification of our data-uri svg images.

More specifically postcss-svgo does not play well with data-uris not containing a semicolon after the mediatype:
data:image/svg+xml,%3Csvg ... will yield this warning Non-whitespace before first tag and incorrectly optimize the svg. For now I managed to make svgo behave correctly by simply adding a semicolon after the mediatype (this should not be necessary according to the data uri scheme). Other solutions could be:

  • Switching off svgo from cssnano all together
  • Decode our svg in utf-8 (e.g. from %3C to < - like what they do in the postcss-svgo example docs)

How to test

To test that the bundle is minified correctly and the confetti animation works as expected you can serve (npx serve) this content in an temporary test.html file at the root level of this repo.

<!DOCTYPE html>
<html lang="en">
<head>
    <meta charset="UTF-8">
    <title>Test</title>
    <link href="dist/css/stacks.min.css" rel="stylesheet">
</head>
    <body>
        <div class="hmn2 bg-confetti-animated">…</div>
    </body>
</html>

No warning should be showed when running npm run build. Tests and Linters should pass.

This PR also updates all remaining dependencies. All minor updates apart from the vitest update which is the reason of why the snapshot files has changed (if you look carefully you can notice that only some backward slashes have been removed which is a good thing and no real regression is happening in the PR).

Here you can find a diff checker link comparing stacks.min.css before and after the PR

@giamir
fix(bundling): correctly generate minfied css bundle
7f9d389 
This change fixes between other things an issue with the confetti animation not working correctly in the minified css bundle
@netlify Netlify
Copy link

netlify bot commented Jan 3, 2024 •
edited
Loading

✅ Deploy Preview for stacks ready!

Name Link
🔨 Latest commit 2c80bea
🔍 Latest deploy log https://app.netlify.com/sites/stacks/deploys/659715f65d0cb90008397dbb
😎 Deploy Preview https://deploy-preview-1595--stacks.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@giamir giamir mentioned this pull request Jan 3, 2024
Closed
@giamir giamir requested a review from dancormier January 3, 2024 17:24
@giamir giamir marked this pull request as ready for review January 3, 2024 17:24
@dancormier
chore(banner): rename PPCPs to better reflect property
2c80bea 
I renamed the PPCPs that provide the value for the axis being translated (based on the syntax described [here](https://developer.mozilla.org/en-US/docs/Web/CSS/transform-function/translate3d#ty)). I think using `x` was a mistake and this commit corrects that and unifies the syntax with the newly-added PPCP.
dancormier
dancormier approved these changes Jan 4, 2024
View reviewed changes
Copy link
Contributor

@dancormier dancormier left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Works well! Thanks for this fix! I'm perfectly happy turning off svgo from css-nano

Note
I made a minor naming change in 2c80bea that should result in no material changes to the build.

@dancormier dancormier merged commit c02802c into develop Jan 4, 2024
5 checks passed
@dancormier dancormier deleted the STACKS-366/fix-broken-svgs-by-minification-process branch January 4, 2024 23:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dancormier dancormier dancormier approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@giamir @dancormier