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

fix: handle 404 on cloud functions #105

Merged
merged 16 commits into from
Jun 28, 2022
Merged

fix: handle 404 on cloud functions #105

merged 16 commits into from
Jun 28, 2022

Conversation

mirestrepo
Copy link
Member

@mirestrepo mirestrepo commented Jun 16, 2022

This one was a bit tricky to figure out so I want to write out what I learned. The solution may not be perfect or the best, but it's the one way I got it to work. The issue arise from the combination of the way Nuxt handles 404, and the fact that our website is not fully static

Options for handling 404 page

If our site was fully static

We would need Firebase to redirect to a 404.html. To accomplish this one, would need to tell Nuxt to generate the 404.html page in the config (we do that) by adding to the nuxt config file:

  generate: {
    fallback: "404.html"
  },

and to add "destination": "/404.html" to the firebase config. We tried the following

      "rewrites": [
         {
           "source": "**",
           "function": "ssrapp",
           "destination": "/404.html"
         }
       ],

At first we thought that worked, but then realized that adding "destination": "/404.html" overshadows the function and the dynamic routes never get hit

Links

Final solution.

The solution is a bit inspired by this post. More specifically by these comments:

Breakdown of the problem

  • Nuxt doesn’t give middlewares an easy way to see if a route is valid or not
  • Nuxt runs its own 404 handler that can’t be intercepted in the middleware chain
  • Nuxt doesn’t have a clearly documented way of overriding the 404 handler on the server side

What we need to do

  • Intercept all traffic and check if the requests are renderable by Nuxt
  • Proxy all other requests

To make things more explicit.

  • All static pages (the ones that come from Nuxt Contex) are hit right away. This is handled by firebase.
  • When a route is not found in the static content (/dist) folder, then the cloud function is called. This is handled by firebase (config - "function": "ssrapp",)
  • In the cloud function I am keeping a list of the dynamic routes that the Nuxt app should handle, every other route is redirected to the static 404.html

If we don't do this and let Nuxt handle the non existing route, we get a 500 instead of 404, and the website becomes unresponsive unless refreshed. I think this is because Nuxt my not treat 404 as an error and therefore our try/catch doesn't handle it appropriately

@github-actions
Copy link

github-actions bot commented Jun 16, 2022

Visit the preview URL for this PR (updated for commit 6942379):

https://qa-ccv-brown-edu--pr105-debug-404-4x87pxyy.web.app

(expires Tue, 05 Jul 2022 04:47:16 GMT)

🔥 via Firebase Hosting GitHub Action 🌎

@mirestrepo mirestrepo changed the title chore: trying a clean branch from main fix: handle 404 on cloud functions Jun 17, 2022
@mirestrepo mirestrepo requested review from mcmcgrath13, eldu and tdivoll and removed request for mcmcgrath13 June 24, 2022 15:34
Copy link

@broarr broarr left a comment

Choose a reason for hiding this comment

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

This looks good to me! Couple of style comments, but nothing that should prevent a merge!

Comment on lines +26 to +27
}
else{
Copy link

Choose a reason for hiding this comment

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

I'd stick this on one line, not that it matters though...

@@ -11,6 +11,8 @@ let isReady = false;

let nuxt = loadNuxt('start');

let dynamicRoutes = ['/_ghapi/status', '/_workday/opportunities']
Copy link

Choose a reason for hiding this comment

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

I'd consider renaming this to whitelistedRoutes, that way we know that these routes are allowed by the cloud function. It's the normal language used for this sort of thing in networking. Obviously this doesn't matter either ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don’t love the whitel/black list terminology, but I could use allowedRoutes or something more clear

Comment on lines +29 to +30
res.set('X-Cascade', 'PASS')
res.status(404).redirect('/404.html')
Copy link

Choose a reason for hiding this comment

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

I think y'all might need a style enforcer like prettier or standard I'm seeing ; sometimes, but not others

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably… we have it in the parent dir… may just need to add it to the functions dir..

Comment on lines +69 to +71
// eslint-disable-next-line no-console
console.log('GH Response')
console.log(response);
Copy link

Choose a reason for hiding this comment

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

You can probably delete this now

await nuxt.server.app.handle(req, res);
}
else{
console.log('Redirect to 404 page');
Copy link

Choose a reason for hiding this comment

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

Do you need this log still?

Copy link
Member Author

Choose a reason for hiding this comment

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

Probably not

Copy link
Contributor

@eldu eldu left a comment

Choose a reason for hiding this comment

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

Thanks you for tackling this weird problem! I didn't realize how weirdly nuxt handles 404! Great learnings in here and thank you for documenting them as well as instructions for updates.

I made comments on some typos. And then Brad also mentioned changes as well. Looking very good Isabel! Thanks again!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@mirestrepo
Copy link
Member Author

Thanks @broarr and @eldu for reviewing! I’ve merged the docs changes since I can do it from my ipad and made #107 to address Brad’s suggestions. I’m gonna merge so this bug doesn’t stay around longer

@mirestrepo mirestrepo merged commit ba0eed9 into main Jun 28, 2022
@mirestrepo mirestrepo deleted the debug-404 branch June 28, 2022 05:03
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