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

build: update and simplify script #326

Open
wants to merge 14 commits into
base: main
Choose a base branch
from

Conversation

uncenter
Copy link
Member

@uncenter uncenter commented Sep 7, 2024

Updates the script to use the more modern web ESM syntax (as opposed to the old Node.js CJS syntax). I've also simplified the script to use Catppuccin's npm package - which is already installed and used in the SASS files to import the colors, only updated the version of it - and use the SASS API for compilation instead of using the CLI as an npm script. I did some updates to the package.json as well (cleaned up the scripts, updated metadata).

@uncenter
Copy link
Member Author

uncenter commented Sep 7, 2024

The only difference in build output now is that the lines of trailing newlines and more importantly the .css files are in just dist/, not dist/dist/. Not sure why they were in dist/dist/ in the first place 🤔

@sgoudham
Copy link
Contributor

sgoudham commented Sep 7, 2024

I don't think you want to change the /dist/dist since that'll break people's URLs?

@uncenter
Copy link
Member Author

uncenter commented Sep 7, 2024

Ah that's why, the GH Pages action deploys the dist directory, and the dist in the dist is needed for the URLs yeah. Argh...

@uncenter
Copy link
Member Author

uncenter commented Sep 7, 2024

My other question is why do we have:

/**
* @name Catppuccin Frappe
* @author winston#0001
* @authorId 505490445468696576
* @version 0.2.0
* @description 🎮 Soothing pastel theme for Discord
* @website https://github.com/catppuccin/discord
* @invite r6Mdz5dpFc
**/

Those comments are just stripped out when we compress the files in build...

@uncenter
Copy link
Member Author

uncenter commented Sep 7, 2024

I guess we can remove them or update the fields later, outside of the scope of this PR.

@ToxicAven
Copy link
Collaborator

On my hardware, this script completes 2-3 seconds slower than the script currently in use. it also removes the release task, which is used by the CI and is not handled here. It also removes the watch and original build tasks, which are incredibly helpful for development. Obviously the latter can be rectified post-merge, but with the script generally performing slower, I'm not sure if that's the play.

@uncenter
Copy link
Member Author

uncenter commented Oct 4, 2024

Hi! Sorry for the delayed response, I've been hard at work resolving that perf issue... 🕵️

I've managed to get the build script, which in this branch is the equivalent of the release script (all accent variations and whatnot) down to 781.1 ms (using yarn build, measured with https://github.com/sharkdp/hyperfine)!

For comparison, the previous release script ran for ~2.055 s, and the previous build script (no accents generated) ran just a teeny bit quicker than this PR's release script at 610.7 ms.

The only difference remaining would be the watch script, which could be resolved with something like https://github.com/eradman/entr maybe? I wouldn't expect a watch mode to be super useful in this port since it isn't doing anything like hot-reloading, though I can see some convenience in not having to run yarn build after edits.

{
style: "compressed",
loadPaths: ["node_modules/", "src/"],
silenceDeprecations: ["color-functions"],
Copy link
Member Author

Choose a reason for hiding this comment

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

This is needed since we are currently using deprecated Sass functions to adjust lightness of colors.

Deprecation Warning: lighten() is deprecated. Suggestions:

color.scale($color, $lightness: 100%)
color.adjust($color, $lightness: 35%)

More info: https://sass-lang.com/d/color-functions

  ╷
9 │   --brand-experiment-230: #{lighten($brand, math.div(70%, 2))};
  │                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  ╵
    ../../../../src/components/_variables.scss 9:29  @import
    _theme.scss 1:9                                  @import
    - 16:9                                           root stylesheet

@uncenter
Copy link
Member Author

Any update on this? Happy to try to resolve these conflicts but if this isn't going to be reviewed in the near future I don't want to waste my time.

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.

3 participants