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

chore(404): no cache for 404 #3226

Merged
merged 29 commits into from
Mar 17, 2023
Merged

chore(404): no cache for 404 #3226

merged 29 commits into from
Mar 17, 2023

Conversation

ovflowd
Copy link
Member

@ovflowd ovflowd commented Mar 16, 2023

This PR adds no-cache for 404 pages

Copy link
Member

@MoLow MoLow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@richardlau
Copy link
Member

@ovflowd

# nginx -t
nginx: [emerg] invalid number of arguments in "try_files" directive in /etc/nginx/sites-enabled/00-nodejs.org:39
nginx: configuration file /etc/nginx/nginx.conf test failed

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

Yeah, that makes sense. It was a bet, let me change that.

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

@richardlau done.

@richardlau
Copy link
Member

# nginx -t
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful

🎉

Going to deploy 🤞 .

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

@richardlau I've just made an update using alias as @targos suggested, not sure which option is better tbh

@richardlau
Copy link
Member

With the alias

# nginx -t
nginx: [emerg] the "alias" directive cannot be used inside the named location in /etc/nginx/sites-enabled/00-nodejs.org:39
nginx: configuration file /etc/nginx/nginx.conf test failed

@targos
Copy link
Member

targos commented Mar 16, 2023

Let's keep it without the alias, then.

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

Wait, if the real path is /en/404/index.html ... shouldn't that actually be causing problems? How is it able to find /en/404.html?

Copy link
Member

@richardlau richardlau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making sure this doesn't land with the alias (failing nginx -t).

@richardlau
Copy link
Member

nginix is still unhappy:

# nginx -t
nginx: [emerg] the "alias" directive cannot be used inside the named location in /etc/nginx/sites-enabled/00-nodejs.org:39
nginx: configuration file /etc/nginx/nginx.conf test failed

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

Yeah, alias cannot be used within named locations. That makes sense.

@richardlau
Copy link
Member

# nginx -t
nginx: the configuration file /etc/nginx/nginx.conf syntax is ok
nginx: configuration file /etc/nginx/nginx.conf test is successful

🎉

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

@richardlau this needs to get merged first nodejs/nodejs.org#5155

(If you apply the nginx config now next.js default 404 is gone, meaning /404.html doesn't exist anymore)

@ovflowd
Copy link
Member Author

ovflowd commented Mar 16, 2023

I've just merged the PR on nodejs/nodejs.org. (@richardlau). Let's wait for the build to finish within the server 👀

@targos
Copy link
Member

targos commented Mar 17, 2023

c7ab832 is now deployed

@targos targos merged commit 90e54ba into nodejs:main Mar 17, 2023
@ovflowd ovflowd deleted the patch-2 branch November 1, 2024 17:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants