- 
          
 - 
                Notifications
    
You must be signed in to change notification settings  - Fork 2.9k
 
perf: phase 1 - compression & image optimization (@abdo-spices) #7014
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
base: master
Are you sure you want to change the base?
Conversation
| }, | ||
| }, | ||
| ViteImageOptimizer(), | ||
| compression({ | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Takes about 26s on my machine, and yeah, the Spanish list file (~1MB) is the main bottleneck at ~80% of compression time.
I think pre-compression is worth it though
latency: whether it's the server or CDN doing compression on-the-fly, you're adding 5-50ms per request. pre-compressed files skip that entirely.
server load (if it's not the CDN): If the server is handling compression instead of the CDN, that's CPU being used on every request. Pre-compression moves that work to build time, which happens way less frequently.
26s at build time vs. guaranteed fast response times without any runtime processing seems like a decent trade-off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think pre-compression is worth it though
latency: whether it's the server or CDN doing compression on-the-fly, you're adding 5-50ms per request. pre-compressed files skip that entirely.
any source on that? I'd hope firebase would cache the compressed file and not compress it each time it is requested.
Besided, how is it working? The plugin generates an additional  .br file for styles, json etc. How is it used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besided, how is it working? The plugin generates an additional .br file for styles, json etc. How is it used?
All files can be compressed not just JS or CSS, but also images and JSON. When the browser detects a .br version, it automatically requests that instead of the original file. This significantly reduces bandwidth usage, which is important since Firebase charges based on bandwidth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firebase is already compressing files on upload. When i check a url that bypasses cloudflare everything is already compressed. So im not sure this is needed.
| "oxlint": "1.15.0", | ||
| "postcss": "8.4.31", | ||
| "sass": "1.70.0", | ||
| "sharp": "0.32.6", | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a  reason for this two  year  old version  of sharp? the current version is  0.34.4
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is bug in modern sharp binary that specific version do not have it so we need 0.32.6
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which bug, please link the source
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
which bug, please link the source
the bug is mainly with build script in sharp i can fix it in my device but well break on other people machines
there is more cases
it also happen on windows, the it's also known bug in astro since a lot of astro projects uses image component that's powered by sharp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Youre linking very old issues here. Are you sure this is a problem?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please delete the replaced   .png files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to keep the original images
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? Image optimization will pick them up and they are not used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why? Image optimization will pick them up and they are not used.
I can delete them, but it's better to keep them we might switch the image optimizer or convert them to other formats later if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think ideally we would keep the originals, but also dont commit the compressed files, only output them to the dist directory when deploying.
| 
           It kind of feels wasteful to run the image optimization for each build instead of doing it once when we add a new image (which is not very often). That said, the overhead is 1s on my machine for this, so this is more a workflow question, @Miodec what is your opinion?  | 
    
          
 not very often that's right but we need to keep the original images  | 
    
| import MagicString from "magic-string"; | ||
| import { Fonts } from "./src/ts/constants/fonts"; | ||
| 
               | 
          ||
| global.navigator = undefined; // sass compatibly & imagemin bug https://github.com/vitejs/vite/issues/5815#issuecomment-984041683 | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Youre linking a closed issues from 4 years ago, are you sure this is needed?


Description
Checks
packages/schemas/src/languages.tsfrontend/src/ts/constants/languages.tsfrontend/static/languagespackages/schemas/src/themes.tsfrontend/src/ts/constants/themes.tsfrontend/static/themespackages/schemas/src/layouts.tsfrontend/static/layoutsfrontend/static/webfontspackages/schemas/src/fonts.tsfrontend/src/ts/constants/fonts.tsCloses #