feat: nginx improvements #1545

Merged
konrad merged 1 commits from dpschen/frontend:feature/nginx-improvements into main 2022-04-02 16:12:47 +00:00
Member

I have not a lot of experience with nginx — see this as foundation for
discussion.

I came up with these changes when going through the vite-plugin-pwa documentation, I think (this is from a stash from a while ago and I didn't want to submit something I wasn't sure of).

I have not a lot of experience with nginx — see this as foundation for discussion. I came up with these changes when going through the vite-plugin-pwa documentation, I think (this is from a stash from a while ago and I didn't want to submit something I wasn't sure of).
dpschen reviewed 2022-02-15 13:09:06 +00:00
nginx.conf Outdated
@ -68,2 +90,3 @@
}
location ~* .(txt|webmanifest|css|js|mjs|map|svg|jpg|jpeg|png|ico|ttf|woff|woff2|wav)$ {
root /usr/share/nginx/html;
Author
Member
See https://www.nginx.com/resources/wiki/start/topics/tutorials/config_pitfalls/#root-inside-location-block
dpschen marked this conversation as resolved
dpschen reviewed 2022-02-15 13:10:06 +00:00
nginx.conf Outdated
@ -42,7 +45,6 @@ http {
# Expires map
map $sent_http_content_type $expires {
default off;
text/html max;
Author
Member

If we cache html, how should the browser know of new changes? Or am I missing something?

If we cache html, how should the browser know of new changes? Or am I missing something?

That's correct. We should not cache the html.

That's correct. We should not cache the html.
dpschen marked this conversation as resolved
dpschen reviewed 2022-02-15 13:10:20 +00:00
nginx.conf Outdated
@ -54,3 +56,3 @@
image/x-icon max;
audio/wav max;
~image/ max;
~images/ max;
Author
Member

This path was wrong

This path was wrong

What about mime types like image/png? Those would not get cached with the new implementation you proposed here, but would get cached with the old one iirc.

What about mime types like `image/png`? Those would not get cached with the new implementation you proposed here, but would get cached with the old one iirc.
Author
Member

Where did it get cached in the old implementation? Not sure where I would have removed that?

Where did it get cached in the old implementation? Not sure where I would have removed that?

IIRC using ~ at the beginning tells nginx to handle this like a regular expression. That would still not cache images I think hmmmm. I'm not even sure what this does. Did you test it?

These may even duplicated since we're already adding a caching header to everything in /assets?

IIRC using `~` at the beginning tells nginx to handle this like a regular expression. That would still not cache images I think hmmmm. I'm not even sure what this does. Did you test it? These may even duplicated since we're already adding a caching header to everything in `/assets`?
Author
Member

I didn't test.
To be honest I'm only guessing here 🤷‍♂️

I didn't test. To be honest I'm only guessing here 🤷‍♂️

Can you test it?

Can you test it?
Author
Member

Yes, but probably my time is better spent reviewing other branches :D
I've almost no experience setting nginx up (mostly I was just messing around with an existing config).

Yes, but probably my time is better spent reviewing other branches :D I've almost no experience setting nginx up (mostly I was just messing around with an existing config).

huh yeah I get that :D

The nginx config in the repo is the one used by the docker file. That means running

docker build . -t vikunja/frontend:dev
docker run -p 10002:80 vikunja/frontend:dev

should build everything and make the frontend accessible on port 10002 on your machine with the new config (at least that's what I used to test it).

Will check tomorrow (or after that) if everything works as expected.

huh yeah I get that :D The nginx config in the repo is the one used by the docker file. That means running ``` docker build . -t vikunja/frontend:dev docker run -p 10002:80 vikunja/frontend:dev ``` should build everything and make the frontend accessible on port 10002 on your machine with the new config (at least that's what I used to test it). Will check tomorrow (or after that) if everything works as expected.
Author
Member

That helps, will try.

That helps, will try.
Member

Hi dpschen!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://1545-feature-nginx-improvements--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi dpschen! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://1545-feature-nginx-improvements--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen reviewed 2022-02-15 13:12:17 +00:00
nginx.conf Outdated
@ -67,1 +69,4 @@
root /usr/share/nginx/html;
# all assets contain hash in filename, cache forever
Author
Member

Not sure if this is a duplication.
I remember also that we had some problems with try_files.

Not sure if this is a duplication. I remember also that we had some problems with `try_files`.

I think this is fine. Back when I initially created the nginx.conf file, we didn't have vite so all assets would go in top-level css/, js/, etc folders, not all bundled up in assets/.

I think this is fine. Back when I initially created the nginx.conf file, we didn't have vite so all assets would go in top-level `css/`, `js/`, etc folders, not all bundled up in `assets/`.
dpschen marked this conversation as resolved
dpschen reviewed 2022-02-15 13:15:39 +00:00
nginx.conf Outdated
@ -14,6 +14,10 @@ http {
include /etc/nginx/mime.types;
default_type application/octet-stream;
types {
Author
Member

See: https://vite-plugin-pwa.netlify.app/deployment/nginx.html#configure-manifest-webmanifest-mime-type
This seems to be necessary, since it returns:

→ curl -s -I -X GET https://try.vikunja.io/manifest.webmanifest | grep content-type -i
content-type: application/octet-stream
x-content-type-options: nosniff
See: https://vite-plugin-pwa.netlify.app/deployment/nginx.html#configure-manifest-webmanifest-mime-type This seems to be necessary, since it returns: ```sh → curl -s -I -X GET https://try.vikunja.io/manifest.webmanifest | grep content-type -i content-type: application/octet-stream x-content-type-options: nosniff ```
dpschen marked this conversation as resolved
dpschen changed title from WIP: feat: nginx improvements to feat: nginx improvements 2022-02-15 13:17:58 +00:00
dpschen requested review from konrad 2022-02-15 13:28:08 +00:00
konrad was assigned by dpschen 2022-02-15 13:28:12 +00:00
konrad reviewed 2022-02-19 21:09:23 +00:00
nginx.conf Outdated
@ -70,4 +93,4 @@
try_files $uri $uri/ =404;
}
location / {

Now there's two location / blocks. This breaks the config:

2022/02/19 21:06:32 [emerg] 30#30: duplicate location "/" in /etc/nginx/nginx.conf:96
nginx: [emerg] duplicate location "/" in /etc/nginx/nginx.conf:96
Now there's two `location /` blocks. This breaks the config: ``` 2022/02/19 21:06:32 [emerg] 30#30: duplicate location "/" in /etc/nginx/nginx.conf:96 nginx: [emerg] duplicate location "/" in /etc/nginx/nginx.conf:96 ```
Author
Member

I removed the old block

I removed the old block
dpschen marked this conversation as resolved
konrad requested changes 2022-02-19 21:13:40 +00:00
konrad left a comment
Owner

Please fix the duplicate location block.

Please fix the duplicate location block.
dpschen force-pushed feature/nginx-improvements from e89b279c0c to 40bf139210 2022-02-20 09:47:54 +00:00 Compare
dpschen reviewed 2022-02-20 09:48:21 +00:00
nginx.conf Outdated
@ -35,3 +38,3 @@
gzip_http_version 1.1;
gzip_min_length 256;
gzip_types text/plain text/css application/json application/x-javascript application/javascript text/xml application/xml application/xml+rss text/javascript application/vnd.ms-fontobject application/x-font-ttf font/opentype image/svg+xml image/x-icon audio/wav;
gzip_types
Author
Member

This stayed the same.
It's just indented

This stayed the same. It's just indented
dpschen marked this conversation as resolved
Author
Member

I used tabs by accident while there where spaces everywhere before.
Shall I add this to .editorconfig, because my editor was just following our defined rules here?

I used tabs by accident while there where spaces everywhere before. Shall I add this to `.editorconfig`, because my editor was just following our defined rules here?
dpschen reviewed 2022-02-20 09:51:06 +00:00
nginx.conf Outdated
@ -77,0 +104,4 @@
try_files $uri /index.html =404;
}
location ~* .(txt|webmanifest|css|js|mjs|map|svg|jpg|jpeg|png|ico|ttf|woff|woff2|wav)$ {
Author
Member

Not sure if this still makes sense

Not sure if this still makes sense

Why wouldn't it?

Why wouldn't it?
Author
Member

Because all those files should be in the assets folder or have their own rule.

Because all those files should be in the assets folder or have their own rule.

I guess it won't hurt to keep them as a backup.

I guess it won't hurt to keep them as a backup.
dpschen reviewed 2022-02-20 09:53:40 +00:00
nginx.conf Outdated
@ -77,0 +101,4 @@
autoindex off;
expires off;
add_header Cache-Control "public, max-age=0, s-maxage=0, must-revalidate" always;
try_files $uri /index.html =404;
Author
Member

I hope this works =)

I hope this works =)

Looks like it :)

Looks like it :)
dpschen marked this conversation as resolved
dpschen force-pushed feature/nginx-improvements from 40bf139210 to 34875e6d73 2022-02-20 11:20:28 +00:00 Compare
Owner

I used tabs by accident while there where spaces everywhere before.
Shall I add this to .editorconfig, because my editor was just following our defined rules here?

I'd go with tabs since we're using them everywhere, but it should be the same for the whole file.

> I used tabs by accident while there where spaces everywhere before. Shall I add this to .editorconfig, because my editor was just following our defined rules here? I'd go with tabs since we're using them everywhere, but it should be the same for the whole file.
Author
Member

I'd go with tabs since we're using them everywhere, but it should be the same for the whole file.

Will convert

> I'd go with tabs since we're using them everywhere, but it should be the same for the whole file. Will convert
dpschen force-pushed feature/nginx-improvements from 34875e6d73 to 99abedb801 2022-02-21 21:26:37 +00:00 Compare
dpschen force-pushed feature/nginx-improvements from 99abedb801 to 680857c7d9 2022-02-21 21:27:05 +00:00 Compare
konrad force-pushed feature/nginx-improvements from 680857c7d9 to a797bdd7d1 2022-04-02 15:53:08 +00:00 Compare
konrad approved these changes 2022-04-02 15:54:21 +00:00
konrad left a comment
Owner

Looks like everything works, merging.

Thanks!

Looks like everything works, merging. Thanks!
konrad merged commit 52fdc2614b into main 2022-04-02 16:12:47 +00:00
konrad deleted branch feature/nginx-improvements 2022-04-02 16:12:47 +00:00
This repo is archived. You cannot comment on pull requests.
No description provided.