-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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(gatsby): add history fallback for client-only routes #11610
Conversation
…es in serve (gatsbyjs#11581)" This reverts commit 75f6118.
…es in serve (gatsbyjs#11581)" This reverts commit 75f6118.
Wanted to get this out before I head out for the day. f999abb contains a simple history mechanism where if the page If no matches are found--will fall through to existing 404 page (which matches existing functionality). Marking this a WIP, because I haven't tested it a ton. Will do more tomorrow. Probably a lot of edge cases here. |
@jquense if you have some time--could you validate with a client-only path example of yours? I reached out to a few people on Discord and don't have a ton of examples. I validated it works with some of our basic ones, but want to ensure it also handles the more "real world" examples. |
@@ -56,7 +56,6 @@ | |||
"eslint-plugin-react": "^7.8.2", | |||
"express": "^4.16.3", | |||
"express-graphql": "^0.6.12", | |||
"express-history-api-fallback": "^2.2.1", |
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.
bye felicia
Totally not a blocker - do you think it's worth adding cy.visit(`/client-only-paths/param`).waitForAPI(`onRouteUpdate`)
cy.getTestElement(`dom-marker`).contains(`param`) |
@pieh I think that's a great idea! I'll do this today! |
@DSchau this looks great! I'll try and validate on our complex app with client routes today |
Just circling back to my comment about e2e - I guess it wouldn't work because if we didn't handle client-only paths in serve, then we would first render 404 page and when app mounts we would re-render client-only-path, so this test would cause just false sense of correctness. Unless there's way to disable JS in cypress for single visit |
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.
) * Revert "fix(gatsby): use history fallback to display client-only routes in serve (gatsbyjs#11581)" This reverts commit 75f6118. * Revert "fix(gatsby): use history fallback to display client-only routes in serve (gatsbyjs#11581)" This reverts commit 75f6118. * fix: use smart history fallback that understands gatsby matchPath * chore: fix * refactor: swap to reach router instead of micromatch * chore: move check up * chore: tweak name
) * Revert "fix(gatsby): use history fallback to display client-only routes in serve (gatsbyjs#11581)" This reverts commit 75f6118. * Revert "fix(gatsby): use history fallback to display client-only routes in serve (gatsbyjs#11581)" This reverts commit 75f6118. * fix: use smart history fallback that understands gatsby matchPath * chore: fix * refactor: swap to reach router instead of micromatch * chore: move check up * chore: tweak name
Description
Quick fix for an issue introduced in #11581 that is causing e2e failures in production_runtime.
The main issue is that the history fallback is not smart enough to understand the underlying page/routing structure of a Gatsby application and is over-eager in handling 404 routes. In other words--it consumes meaningful 404 errors that we use in our e2e tests.
The longer term fix that @pieh proposed is to tie into.cache/pages.json
and intelligently match on client-only routes (usingmatchPath
) which would check thereq.url
against the matchPath pattern. If it matches, it should serve theindex.html
route, otherwise it should fallback to 404 behavior.Implemented in f999abb