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

feat: gzip js, css, SVG icons assets #11208

Merged
merged 4 commits into from
Jan 8, 2025
Merged

feat: gzip js, css, SVG icons assets #11208

merged 4 commits into from
Jan 8, 2025

Conversation

stephanegigandet
Copy link
Contributor

@stephanegigandet stephanegigandet commented Jan 8, 2025

As pointed by @github-throwaway in openfoodfacts/smooth-app#5583 (comment) , we currently serve uncompressed assets (CSS, JS etc.).

We should add .gz versions so that they can be served by nginx. We need to keep the non .gz files as well as we use try_files, which returns a 404 if the uncompressed file does not exist:

                try_files resources/$uri $uri $uri/ =404;
                gzip_static always;
                gunzip on;

@stephanegigandet stephanegigandet requested a review from a team as a code owner January 8, 2025 13:30
@github-actions github-actions bot added the dependencies Pull requests that update a dependency file label Jan 8, 2025
@stephanegigandet stephanegigandet changed the title fix: gzip js, css assets - WIP feat: gzip js, css, SVG icons assets Jan 8, 2025
@stephanegigandet
Copy link
Contributor Author

Works locally, but in GitHub tests I get this error:

#22 [dynamicfront builder 13/13] RUN npm run build
#22 0.329 
#22 0.329 > [email protected] build
#22 0.329 > gulp
#22 0.329 
#22 0.885 [14:18:40] Requiring external module ts-node/register
#22 1.955 TypeError: Unknown file extension ".ts" for /opt/product-opener/gulpfile.ts
#22 1.955     at Object.getFileProtocolModuleFormat [as file:] (node:internal/modules/esm/get_format:218:9)
#22 1.955     at defaultGetFormat (node:internal/modules/esm/get_format:[244](https://github.com/openfoodfacts/openfoodfacts-server/actions/runs/12672402831/job/35316357560?pr=11208#step:4:245):36)
#22 1.955     at defaultLoad (node:internal/modules/esm/load:122:22)
#22 1.955     at async ModuleLoader.load (node:internal/modules/esm/loader:570:7)
#22 1.955     at async ModuleLoader.moduleProvider (node:internal/modules/esm/loader:443:45)
#22 1.955     at async ModuleJob._link (node:internal/modules/esm/module_job:106:19) {
#22 1.955   code: 'ERR_UNKNOWN_FILE_EXTENSION'
#22 1.955 }
#22 ERROR: process "/bin/sh -c npm run build" did not complete successfully: exit code: 1

There's a huge thread about this error here: TypeStrong/ts-node#1997

Copy link

sonarqubecloud bot commented Jan 8, 2025

@teolemon
Copy link
Member

teolemon commented Jan 8, 2025

I was convinced we had already enabled that in the past @stephanegigandet

@alexgarel alexgarel merged commit c0a5275 into main Jan 8, 2025
14 checks passed
@alexgarel alexgarel deleted the gzip-js-css branch January 8, 2025 18:15
@stephanegigandet
Copy link
Contributor Author

@teolemon it probably was working before because we had the nginx gzip module enabled in https://github.com/openfoodfacts/openfoodfacts-server/blob/main/conf/nginx/nginx.conf

but that file is not used anymore in production apparently, not sure if it's by design or not.

@stephanegigandet
Copy link
Contributor Author

I'm not sure if it's intended and when it changed, but in production the nginx.conf is not the one we have in github.
One of the differences is that gzip_types is not set, so we don't compress anything else than html.

root@off:/etc/nginx# diff /srv/off/conf/nginx/nginx.conf nginx.conf
7c7
< 	worker_connections 768;
---
> 	worker_connections 4096;
19,20d18
< 	tcp_nodelay on;
< 	keepalive_timeout 65;
34c32
< 	ssl_protocols TLSv1 TLSv1.1 TLSv1.2 ; # Dropping SSLv3, ref: POODLE
---
> 	ssl_protocols TLSv1 TLSv1.1 TLSv1.2 TLSv1.3; # Dropping SSLv3, ref: POODLE
49d46
< 	gzip_disable "msie6";
56c53
< 	gzip_types text/plain text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript text/csv;
---
> 	# gzip_types text/plain text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript;
64,65d60
<
< 	client_max_body_size 20M;

@stephanegigandet
Copy link
Contributor Author

That being said, it's better to have pre-compressed static assets I think. We could remove the try_files (I'm not sure they are really useful, it doesn't make much sense to me to try multiple locations...)

                # First attempt to serve request from resource, then as file,
                # then as directory, then fall back to displaying a 404.
                try_files resources/$uri $uri $uri/ =404;
                gzip_static always;
                gunzip on;

If we remove the try_files, then we don't need to keep the uncompressed assets, as gunzip on will work.

alexgarel added a commit that referenced this pull request Jan 10, 2025
🤖 I have created a release *beep* *boop*
---


##
[2.54.0](v2.53.0...v2.54.0)
(2025-01-10)


### Features

* gzip js, css, SVG icons assets
([#11208](#11208))
([c0a5275](c0a5275))


### Bug Fixes

* "NutriScore V2" SVGs with blank space
([#11218](#11218))
([38d79e8](38d79e8))
* avoid crash in display_orgs_table when org.created_t is not set, fix
permission
([#11203](#11203))
([765d796](765d796))
* greenscore attribute with old ecoscore_data
([#11212](#11212))
([4f596ad](4f596ad))
* Invalid OFF dark icon
([#11206](#11206))
([f63daa8](f63daa8))
* Nutripatrol url trailing slash removal regex
([#11204](#11204))
([f42f8dd](f42f8dd))
* Nutriscore for fresh herbs
([#11112](#11112))
([cdd7cf5](cdd7cf5))
* Remove irrelevant things in Config_obf.pm
([c5d448d](c5d448d))
* Remove irrelevant things in the Open Beauty Facts config
([#11195](#11195))
([c5d448d](c5d448d))
* remove warning in Display.pm related to Environmental Scoring
([#11172](#11172))
([176fe9e](176fe9e))
* Removing irrelevant fields in the Open Products Facts config
([4583ed2](4583ed2))
* Removing irrelevant fields in the Open Products Facts config
([#11202](#11202))
([4583ed2](4583ed2))
* typo in ecoscore redirect
([#11213](#11213))
([85fd575](85fd575))
* use ecoscore data when greenscore not available
([#11197](#11197))
([5ae1273](5ae1273))
* warnings in producers tests
([#11190](#11190))
([0588976](0588976))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

Co-authored-by: Alex Garel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants