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(gatsby): add history fallback for client-only routes #11610

Merged
merged 8 commits into from
Feb 11, 2019

Conversation

DSchau
Copy link
Contributor

@DSchau DSchau commented Feb 6, 2019

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 (using matchPath) which would check the req.url against the matchPath pattern. If it matches, it should serve the index.html route, otherwise it should fallback to 404 behavior.

Implemented in f999abb

@DSchau DSchau requested a review from a team as a code owner February 6, 2019 23:46
@DSchau
Copy link
Contributor Author

DSchau commented Feb 7, 2019

Wanted to get this out before I head out for the day. f999abb contains a simple history mechanism where if the page matchPath matches the requested route, it will serve the index route.

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.

@DSchau
Copy link
Contributor Author

DSchau commented Feb 7, 2019

@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.

@DSchau DSchau removed the status: WIP label Feb 7, 2019
@DSchau DSchau changed the title fix(gatsby): revert inclusion of history middleware fix(gatsby): add history fallback for client-only routes Feb 7, 2019
@@ -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",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bye felicia

@pieh
Copy link
Contributor

pieh commented Feb 8, 2019

Totally not a blocker - do you think it's worth adding serve tests to production-runtime e2e tests? We have client-only-path there https://github.com/gatsbyjs/gatsby/blob/master/e2e-tests/production-runtime/src/pages/client-only-paths.js already so it potentially could be just matter of verifying:

cy.visit(`/client-only-paths/param`).waitForAPI(`onRouteUpdate`)
cy.getTestElement(`dom-marker`).contains(`param`)

@DSchau
Copy link
Contributor Author

DSchau commented Feb 8, 2019

@pieh I think that's a great idea! I'll do this today!

@jquense
Copy link
Contributor

jquense commented Feb 8, 2019

@DSchau this looks great! I'll try and validate on our complex app with client routes today

@pieh
Copy link
Contributor

pieh commented Feb 8, 2019

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

Copy link
Contributor

@wardpeet wardpeet left a comment

Choose a reason for hiding this comment

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

Lets merge!

@DSchau lets create an other PR with a fix to resolve @pieh comment.

@wardpeet wardpeet merged commit a0da7a2 into gatsbyjs:master Feb 11, 2019
@DSchau DSchau deleted the gatsby/serve-404-fix branch February 13, 2019 16:00
gurpreet-hanjra pushed a commit to gurpreet-hanjra/gatsby that referenced this pull request Feb 14, 2019
)

* 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
pragmaticpat pushed a commit to pragmaticpat/gatsby that referenced this pull request Apr 28, 2022
)

* 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
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.

None yet

4 participants