vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

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
Conversation 4 Commits 1 Files Changed 1 +88 -57
dpschen commented 2022-02-15 13:08:03 +00:00
Member
Copy Link

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;
dpschen commented 2022-02-15 13:09:06 +00:00
Author
Member

See https://www.nginx.com/resources/wiki/start/topics/tutorials/config_pitfalls/#root-inside-location-block

See https://www.nginx.com/resources/wiki/start/topics/tutorials/config_pitfalls/#root-inside-location-block
✅ 1
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;
dpschen commented 2022-02-15 13:10:06 +00:00
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?
konrad commented 2022-02-19 21:10:03 +00:00
Owner

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;
dpschen commented 2022-02-15 13:10:20 +00:00
Author
Member

This path was wrong

This path was wrong
konrad commented 2022-02-19 21:11:12 +00:00
Owner

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.
dpschen commented 2022-02-20 09:33:26 +00:00
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?
konrad commented 2022-02-20 16:29:39 +00:00
Owner

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`?
dpschen commented 2022-02-20 21:17:14 +00:00
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 🤷‍♂️
konrad commented 2022-02-20 21:34:49 +00:00
Owner

Can you test it?

Can you test it?
dpschen commented 2022-02-21 21:21:49 +00:00
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).
😆 1
konrad commented 2022-02-21 21:38:49 +00:00
Owner

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.
👍 1
dpschen commented 2022-02-21 21:42:40 +00:00
Author
Member

That helps, will try.

That helps, will try.
frederick commented 2022-02-15 13:10:54 +00:00
Member
Copy Link

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
dpschen commented 2022-02-15 13:12:17 +00:00
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`.
konrad commented 2022-02-19 21:13:06 +00:00
Owner

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/`.
👍 1
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 {
dpschen commented 2022-02-15 13:15:39 +00:00
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 ```
✅ 1
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 / {
konrad commented 2022-02-19 21:09:23 +00:00
Owner

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 ```
✅ 1
dpschen commented 2022-02-20 09:51:41 +00:00
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.
✅ 1
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
dpschen commented 2022-02-20 09:48:21 +00:00
Author
Member

This stayed the same.
It's just indented

This stayed the same. It's just indented
✅ 1
dpschen marked this conversation as resolved
dpschen commented 2022-02-20 09:49:40 +00:00
Author
Member
Copy Link

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)$ {
dpschen commented 2022-02-20 09:51:06 +00:00
Author
Member

Not sure if this still makes sense

Not sure if this still makes sense
konrad commented 2022-02-20 16:30:41 +00:00
Owner

Why wouldn't it?

Why wouldn't it?
dpschen commented 2022-02-20 21:15:11 +00:00
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.
konrad commented 2022-02-20 21:34:00 +00:00
Owner

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

I guess it won't hurt to keep them as a backup.
✅ 1
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;
dpschen commented 2022-02-20 09:53:40 +00:00
Author
Member

I hope this works =)

I hope this works =)
konrad commented 2022-02-20 16:36:30 +00:00
Owner

Looks like it :)

Looks like it :)
👍 1
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
konrad commented 2022-02-20 16:30:27 +00:00
Owner
Copy Link

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.
✅ 1
dpschen commented 2022-02-20 17:17:29 +00:00
Author
Member
Copy Link

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
👍 1
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!
🎉 1
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.
Reviewers
No reviewers
konrad
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
konrad
3 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/frontend#1545
No description provided.
Delete Branch "dpschen/frontend:feature/nginx-improvements"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?