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

Workshop re-directs #169

Merged
merged 6 commits into from
Oct 11, 2023
Merged

Workshop re-directs #169

merged 6 commits into from
Oct 11, 2023

Conversation

macdonst
Copy link
Contributor

@macdonst macdonst commented Oct 5, 2023

This PR adds a catch-all at the root of enhance.dev. So far, it is only populated with 3 re-directs.

  '/backend-workshop': '/workshop',
  '/learn-backend': '/workshop',
  '/backend': '/workshop',

The catch-all route will increment each request of those pages to the views table on enhance.dev. This way we can track which poster was the most popular as a back up to our AWS logs.

The /workshop page has been copied from https://enhance.dev/waitlist and is not properly styled. I believe it uses the old Enhance Styles way of doing things.

I just wanted something there so I could test the functionality end to end. As well, we can probably re-use the email collection form. We can assume any new emails are folks interested in the workshop.

@ryanbethel
Copy link
Contributor

ryanbethel commented Oct 5, 2023

You might want to do 302 instead of 301 redirects for this. I could see us wanting to reuse something like /backend in the future for something else and with 301 it would be cached everywhere.

Also, if most of these are root key words it might be better to use $redirect.mjs rather than $$.mjs. That would leave the catchall for future use.

@ryanbethel
Copy link
Contributor

Another option if you want to support redirects of any path depth is just add:

// /app/api/$redirect.mjs
import get from './$$.mjs'
export get

That will stop the root redirects from accidentally being overridden in the future.

@macdonst
Copy link
Contributor Author

macdonst commented Oct 5, 2023

@ryanbethel good catch, I was rushing to get this done before having to take off at lunch and screwed up the redirect code.

Also, good suggestions on making this re-usable in other routes.

Copy link
Member

@colepeters colepeters left a comment

Choose a reason for hiding this comment

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

Just one suggestion from me! Redirect logic isn't my forte so I'mma presume others are looking at that more closely than me. Also presuming the workshop page will be fully replaced by @kristoferjoseph so whatever's there now is fine I think ;)


export async function checkRedirects(req) {
const path = req.requestContext.http.path
const isPath = Object.keys(redirects).includes(path)
Copy link
Member

Choose a reason for hiding this comment

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

Extremely minor: maybe isRedirectPath for clarity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whenever I see a method that starts with is I think it will return a boolean. In this case, the method will redirect the user if the path requested is in the redirects object.

@macdonst
Copy link
Contributor Author

macdonst commented Oct 6, 2023

@colepeters yes, the workshop page is only a place holder until @kristoferjoseph gets to it.

Signed-off-by: macdonst <[email protected]>
Signed-off-by: macdonst <[email protected]>
Signed-off-by: macdonst <[email protected]>
@macdonst macdonst merged commit 8fc778a into main Oct 11, 2023
@macdonst macdonst deleted the workshop branch October 11, 2023 21:42
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.

3 participants