Properly return 404 when the file does not exist #966

Merged
konrad merged 2 commits from LordGaav/frontend:feature/do-not-return-200-on-every-request into main 2021-11-10 21:38:08 +00:00
Contributor

Nginx does not properly return a 404 code when a file does not exist, causing the service worker and/or browser caching to get confused at times. See my comment on this issue for details:

vikunja/api#966 (comment)

Nginx does not properly return a 404 code when a file does not exist, causing the service worker and/or browser caching to get confused at times. See my comment on this issue for details: https://kolaente.dev/vikunja/api/issues/966#issuecomment-18671
LordGaav added 1 commit 2021-11-09 18:45:18 +00:00
Properly return 404 when the file does not exist.
All checks were successful
continuous-integration/drone/pr Build is passing
f18d63f76d
konrad requested changes 2021-11-09 23:02:39 +00:00
@ -72,3 +72,3 @@
location / {
root /usr/share/nginx/html;
try_files $uri $uri/ /;
try_files $uri $uri/ =404;

Unfortunately this breaks accessing something like /login (or password reset routes, share links etc) directly without going through /.

Unfortunately this breaks accessing something like `/login` (or password reset routes, share links etc) directly without going through `/`.
Author
Contributor

I see, you're right. Unfortunately, when serving with Nginx, the problem that I mentioned in the opening exists (serving 200 OK when actually it should be 404 Not Found).

Is there a way to generate static HTML files for the routes that we need?

I see, you're right. Unfortunately, when serving with Nginx, the problem that I mentioned in the opening exists (serving 200 OK when actually it should be 404 Not Found). Is there a way to generate static HTML files for the routes that we need?

Is there a way to generate static HTML files for the routes that we need?

Maybe, kind of. You could theoretically parse route definitions at router/index.js to figure out a regex map of all routes that are actually required and then put that in the nginx config. But then we might as well think about SSR...

> Is there a way to generate static HTML files for the routes that we need? Maybe, kind of. You could theoretically parse route definitions at `router/index.js` to figure out a regex map of all routes that are actually required and then put that in the nginx config. But then we might as well think about [SSR](https://vitejs.dev/guide/ssr.html)...
Author
Contributor

Maybe we're going about this wrong; we want to avoid returning 200 OK on .js files because that breaks the service worker, any other route doesn't really matter.

Maybe we're going about this wrong; we want to avoid returning 200 OK on .js files because that breaks the service worker, any other route doesn't really matter.

mhhhhh nginx can do regex, I'm sure we could modify the location block to do the try_files only work on files that are not ending on .js. Then add another one to keep serving .js files.

mhhhhh nginx can do regex, I'm sure we could modify the `location` block to do the `try_files` only work on files that are not ending on `.js`. Then add another one to keep serving `.js` files.
Author
Contributor

Indeed, see my latest commit. I applied this to my own installation, and everything still seems to work.

Indeed, see my latest commit. I applied this to my own installation, and everything still seems to work.

Looks good.

Looks good.

Seems error-prone to list the files individually. E.g. mp3 is missing.

Seems error-prone to list the files individually. E.g. `mp3` is missing.
Author
Contributor

I did not actually find any mp3 files in the deployed Docker container, this list is all the extensions that I did find.

I did not actually find any mp3 files in the deployed Docker container, this list is all the extensions that I did find.

@dpschen do you know a way to generate that list?

@dpschen do you know a way to generate that list?

I know that it's possible to prerender which also generates the routes that are static: https://vite-plugin-ssr.com/pre-rendering
Not sure if that helps though.

Regarding the list: I found this after a built in the dist folder using this:

css
html
ico
jpg
js
map
mjs
mp3
png
svg
txt
webmanifest
woff2
I know that it's possible to prerender which also generates the routes that are static: https://vite-plugin-ssr.com/pre-rendering Not sure if that helps though. Regarding the list: I found this after a built in the dist folder using [this](https://stackoverflow.com/a/1842270/15522256): ``` css html ico jpg js map mjs mp3 png svg txt webmanifest woff2 ```
LordGaav added 1 commit 2021-11-10 21:33:12 +00:00
Route all static files except HTML directly to disk or return 404
All checks were successful
continuous-integration/drone/pr Build is passing
71ec28b590
konrad approved these changes 2021-11-10 21:37:47 +00:00
konrad left a comment
Owner

Seems to work, will merge once the CI passes.

Seems to work, will merge once the CI passes.
konrad merged commit 052cd36085 into main 2021-11-10 21:38:08 +00:00
konrad deleted branch feature/do-not-return-200-on-every-request 2021-11-10 21:38:08 +00:00
Author
Contributor

👌

👌
This repo is archived. You cannot comment on pull requests.
No description provided.