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): remove 404 pagedata logic on client side pages #17412

Merged
merged 7 commits into from
Sep 25, 2019

Conversation

wardpeet
Copy link
Contributor

@wardpeet wardpeet commented Sep 5, 2019

Description

We have got a few complaints about page-date returning 404s when doing client side only pages. We fetched page-data of every page and when it returned a 404 we fallback to matchpath.

This pr writes pages that are inside a matchpath to matchpath.json so we can safely rely on match-path.json.

It reverts most of #15762 which was added to do the 404 page-data checks.

Still on my todo is to add a simple test for the findPath function.

Related Issues

Fixes #16097

@wardpeet wardpeet changed the title Fix/404 pagedata fix(gatsby): remove 404 pagedata logic on client side pages Sep 5, 2019
@wardpeet wardpeet marked this pull request as ready for review September 5, 2019 15:53
@wardpeet wardpeet requested a review from a team as a code owner September 5, 2019 15:53
@wardpeet
Copy link
Contributor Author

wardpeet commented Sep 5, 2019

looks like tests aren't passing yet

@wardpeet
Copy link
Contributor Author

wardpeet commented Sep 6, 2019

I've tested it manually and our e2e tests cover it basically pretty well.

I've published a canary version on gatsby@page-data-404

@sidharthachatterjee
Copy link
Contributor

This looks great overall to me! Will hold off on merging this in for now since you've added the needs manual testing label

@oorestisime
Copy link
Contributor

Not sure if this is helpful but i tried this with a project that was impacted by these 404s (and limiting page speeds) and problems "vanished". thanks for working on this :)

@alampros
Copy link
Contributor

Just tested this on a project with gatsby-plugin-offline and it has definitely helped (thank you!). Without it, the 404s were throwing the app into an unrecoverable state while offline.

@wardpeet
Copy link
Contributor Author

It seems like there are some troubles with prefetching page-data that is still happening, I can't really reproduce it so I would suggest merging this one and I'll ask for reproduction for a follow-up fix.

@wardpeet wardpeet added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Sep 25, 2019
Copy link
Contributor

@sidharthachatterjee sidharthachatterjee left a comment

Choose a reason for hiding this comment

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

Looks good!

@gatsbybot gatsbybot merged commit 6c64948 into gatsbyjs:master Sep 25, 2019
@JustFly1984
Copy link

JustFly1984 commented Sep 28, 2019

@wardpeet I have issues with / page and localized copies /ru/ and /en/, rendering as 404 page after this PR was merged and published in 2.15.23

All other pages rendering properly. rolling back to 2.15.22 solves an issue.

I have created this issue: #17980

@antoinerousseau
Copy link
Contributor

antoinerousseau commented Sep 29, 2019

Same here, had to rollback to 2.15.22, otherwise pages created with createPage() are not accessible and land to 404 instead.

@metamn
Copy link

metamn commented Oct 3, 2019

Hmmm ... rolled back to 2.15.22 but still this particular post gives 404: http://metamn.io/react/react-follows-the-functional-reactive-programming-paradigm/

Try to refresh it: First the page is rendered then it is replaced by the 404.

I've done everything: empty cache, renamed the post twice, upgraded all packages ... and nothing.

Thanks!

@metamn
Copy link

metamn commented Oct 3, 2019

If it helps: the 404 comes from page-data.json

Screenshot from 2019-10-03 03-02-28

@mi-na-bot
Copy link
Contributor

@metamn Do you have a matchPath or client only route to provide the 404 page? Can you share the contents of .cache/match-paths.json in your project?

@metamn
Copy link

metamn commented Oct 3, 2019

@Sever1an For the first question: I don't know but here is the repo: https://github.com/metamn/react
For the second:

[]

@mi-na-bot
Copy link
Contributor

@metamn Probably not related to the issue reported above by JustFly1984.

A brief examination reveals that the page is attempting to fetch the page-data.json from an incorrect url: http://metamn.io/react/page-data/-follows-the-functional-reactive-programming-paradigm/page-data.json

The page-data.json does exist at the correct url: http://metamn.io/react/page-data/react-follows-the-functional-reactive-programming-paradigm/page-data.json

@metamn
Copy link

metamn commented Oct 3, 2019

Thanks @Sever1an !!

Any hints why is that? Cleaning cache would help?

@metamn
Copy link

metamn commented Oct 3, 2019

Or should I file a separate issue?

@mi-na-bot
Copy link
Contributor

mi-na-bot commented Oct 3, 2019

I think it would be helpful to create a new issue referencing your suspicion that it started with this pull request and sharing the insight about searching at the wrong url for a page-data.json that exists.

If possible please create a tag or branch of a version of the site known to cause the problem and reference it too.

The Gatsby core team is at a conference and not available this week so keeping things as well structured and clean as possible here will help tremendously.

@metamn
Copy link

metamn commented Oct 3, 2019

Thanks again! :)

@mi-na-bot
Copy link
Contributor

@antoinerousseau Do you have a github of the affected project? Do you have anything in .cache/match-paths.json after a build?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Requests for page-data.json return 404 in client side routes
9 participants