commercetools-docs-kit icon indicating copy to clipboard operation
commercetools-docs-kit copied to clipboard

404 page fails to load javascript

Open industrian opened this issue 2 years ago • 16 comments

I've noticed what appears to be a bug on 404 pages. Tested in both Chrome and Safari.

  • Search cannot be used (clicking the search field does nothing)
  • Left-side navigation cannot be expanded.

Link to 404 page.

industrian avatar Oct 18 '23 11:10 industrian

The megamenu also does not seem to open on 404 pages as well.

industrian avatar Oct 18 '23 15:10 industrian

It might be related to this issue. Needs further investigation

gabriele-ct avatar Oct 19 '23 08:10 gabriele-ct

After some tests here is what I've found out:

  • When opening a microsite non existing page I see something like this:

Image

Essentially the first 404 is returned by the microsite 404 rule (which works as expected):

{
      src: `/${workingDirectoryName}/(.*)`,
      status: 404,
      headers: { "x-custom-404": "microsite404" }, // custom header added for debug
      dest: `/${workingDirectoryName}/404`,
    },

When the 404 page loads, the gatsby code tries to load various resources. In the screenshot you can see the code tries to load https://docs.commercetools.com/getting-started/page-data/ss/page-data.json which returns a 404, that's fine too and it's handled by gatsby correcly.

What's wrong is the fact that the code tries also to load https://docs.commercetools.com/ss (note the HEAD request) which matches the catchall rule BUT doesn't return a 404 but a 200. It seems like this rule:

{
      // any other 404 goes to the "docs" homepage 404
      src: '/(.*)',
      status: 404,
      dest: `https://docs.commercetools.com/docs/404`,
    },

gets executed but it doesn't send 404 error.

That somehow causes the issue that surfaces with an internal Gatsby error.

  • Removing the catchall rule resolve the problem with the microsites but any 404 page in the proxy will cause the vercel 404 page to be displayed

gabriele-ct avatar Nov 06 '23 12:11 gabriele-ct

This is how the request would look like without the catchall:

Image

as you can see, the fact that the https://docs-proxy-ga-test-404.commercetools.vercel.app/sssd actually returns a 404 error, seems to fix the issue.

gabriele-ct avatar Nov 06 '23 12:11 gabriele-ct

@nkuehn can you have a look to my previous 2 comments and let me know if that's expected and if would be acceptable to just remove the catchall 404 rule?

gabriele-ct avatar Nov 06 '23 12:11 gabriele-ct

Just a reminder @nkuehn when you have time ☝

gabriele-ct avatar Nov 09 '23 10:11 gabriele-ct

Googled it for gatsyby first before burning time on network traces.

The problem inside a microsite a known bug and they don't care at all https://github.com/gatsbyjs/gatsby/issues/32142 - there is even a one-line fix available from the community: https://github.com/gatsbyjs/gatsby/pull/34237/files#diff-177ac96ca321818b0022cdaa9fda344808eed92150a112f28ff205cda29b8049L606

Outside an existing microsite it's a bit weirder, I think we're not even returning 404 at all. I am getting the 404 page content, but with a 200 status code. Apart from that it has the same bug as the "inside a microsite" case. A 200 is problematic because search crawlers then think this is an actual page location. -> an actual bug on our side vs. a gatsby bug.

I had hoped we can work around it by sending a "Location" header together with the 404 response to redirect but that is not specified in HTTP, Location header is only valid on redirect status codes.

A "dumb but working temporary mitigation" could be to have the 404 page have a piece of javascript that redirects to the home page after 5 seconds. At least people would be practically "unstuck" and the search would work again without having to type into the URL bar. It's not a good web practice but I don't really see gatsby maintainers do anything here. (or, you want to apply a patch to the gatsby codebase via yarn but that would require maintaining it etc. )

nkuehn avatar Nov 09 '23 14:11 nkuehn

Thank you @nkuehn , following your advice i created a PR introducing a new <PageRedirection> component which can be used directly in .mdx files. By default it redirects to / after 5s but it can be configured in case we need it in other context.

gabriele-ct avatar Nov 10 '23 12:11 gabriele-ct

Thank you for making it a generic component from the beginning!

I would suggest to nevertheless keep the ticket open and mark it blocked by the Gatsby issue.

nkuehn avatar Nov 10 '23 13:11 nkuehn

The mitigation fix will be deployed to production soon

gabriele-ct avatar Nov 13 '23 08:11 gabriele-ct

It turns out that the fact React doesn't get loaded (and in general any javascript), it prevents the redirection (which is javascript based) to happen (doh...). So, back to the drawing board. IMHO, since the gatsby fix is unlikely to happen, removing the catchall rule could be the way to go:

  • no maintainance cost for us (compared to applying the fix through yarn)
  • the docs site will be degraded to the default 404 vercel page, but all the rest of website will be fixed.

gabriele-ct avatar Nov 13 '23 10:11 gabriele-ct

could have thought about that... Even if we hardwire it a raw javascript into the static generated file ( https://github.com/commercetools/commercetools-docs-kit/blob/main/packages/gatsby-theme-docs/gatsby-ssr.js ) it would only run when the page is fully loaded and not when navigating inside the microsite. All a mess, damn.

How about going back to patching gatsby through yarn patch? It's a one-line change ( https://github.com/gatsbyjs/gatsby/pull/34237/files#diff-177ac96ca321818b0022cdaa9fda344808eed92150a112f28ff205cda29b8049L606 ) so it's likely git reliably patches it even in newer versions and the file does not even change a lot ( history: https://github.com/gatsbyjs/gatsby/commits/master/packages/gatsby/cache-dir/loader.js )

I would not even do it in the kit but only in the docs repo (we may have to update it with gatsby version changes, so it should have to be done in as few places as possible. If the error remains in the mc app kit website I can live with that.).

nkuehn avatar Nov 13 '23 12:11 nkuehn

I'll give it a try

gabriele-ct avatar Nov 13 '23 12:11 gabriele-ct

@nkuehn It seems the patching is working as expected, please have a look to https://github.com/commercetools/commercetools-docs/pull/4238

gabriele-ct avatar Nov 13 '23 14:11 gabriele-ct

Patching gatsby seems to break at the moment of merging the feature branch into main:

https://github.com/commercetools/commercetools-docs/actions/runs/6878197802

Further investigation is needed

gabriele-ct avatar Nov 16 '23 11:11 gabriele-ct

oh, that's a pity. The error logs reads more like an issue with the UI-Kit dependencies though (but you never know what side effects could be going on)

nkuehn avatar Nov 16 '23 14:11 nkuehn