-
Notifications
You must be signed in to change notification settings - Fork 3
chore: Add redirects #271
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: Add redirects #271
Conversation
Added function to generate redirects. Migrated the current redirects into a JSON file and cleaned up next.config.mjs Added message when a post doesn't exist.
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
One thing to note is I have done a wildcard redirect for /tag/ just in case we ever plan to use tags in the future. |
|
For reference I will remove the script but I have put this into a gist here - https://gist.github.com/colinmurphy/77aa568da3a16341f4fc4a66384924bb |
kellenmace
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for helping us with this, @colinmurphy! My feedback:
I opened this PR expecting to see solely updates to next.config.mjs to accommodate all the redirects we need. I think a simple list of redirects in that file is the approach I'd prefer.
I'm not clear on:
- Why the
generate-redirects-file.jsexists in this PR. This looks like a script that could have been useful to programmatically generate some of the redirects, but not something we want to become a permanent part of the codebase. - Why the array of JavaScript objects that were in
next.config.mjsneeded to be converted to JSON and moved to other files, only to have to read those files back in viafsand parse the JSON back into an array of JavaScript objects (what they were in the first place).
It seems to me that simply having an array of JS objects in next.config.mjs is the cleanest option. it could be within the nextConfig object, or as a const at the bottom of the file– either way. What do you think? Thanks.
Thanks @kellenmace for the review
I was also following what @moonmeister stated in #260 about So I will review the PR now and see which option (file or in the next.config.mjs) makes it easy to read and update. |
1. Removed script 2. Merged both redirect files into next.config.mjs
|
Updated the PR as follows.
|
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
@moonmeister @kellenmace @Fran-A-Dev Updated the PR as per feedback from @moonmeister
Let me know if you need any changes. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
Check out the recent updates to your Headless Platform preview environment:
Learn more about preview environments in our documentation. |
|
I did some basic testing on the preview site
|
What does this do