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

[gatsby] WIP Fix 404 page rendered with missing props and without correct location.pathname #5583

Closed

Conversation

Undistraction
Copy link
Contributor

@Undistraction Undistraction commented May 28, 2018

Currently no props (other than location.pathname are passed in to a custom 404 page, causing lots of potential errors when components used in the layout or in the page itself rely on props. This came to light in #5498 which highlighted the problem, but has lots of potential to effect projects that don't use styled components as well. The current implementation also overwrites props.location.pathname, meaning the path of the page that triggered the 404 is obliterated and is not available to the 404 page, so a message like 'There was no page found at /nopeis not possible because no matter what path triggered the 404,location.pathnameis incorrectly forced to/404`. This is doubly wrong because the url will still reflect the bad path.

It seems that there is duplicate handling of 404 pages both in gatsby/cache-dir/production-app and in gatsby/cahce-dir/component-renderer. Both check to see if the page is available, however the check that happens in production-app results in hardcoding of the 404.html path, whereas the check that happens in component-renderer only results in the rendering of the 404 page, leaving props.location.pathname intact. In short, the check performed in component-renderer is enough on its own, so the extra check (which also has the negative side-effects of 1. obliterating the missing page path and 2. obliterating all props that the layout or other page components might be relying on) is unnecessary.

This PR removes that check. I have verified that this fixes #5498 and also resolves #5544 because the path is now available at props.location.pathname.

closes #5498
closes #5544

@Undistraction Undistraction changed the title fix: 404 page rendered with missing props [gatsby] Fix 404 page rendered with missing props and without correct location.pathname May 28, 2018
@gatsbybot
Copy link
Collaborator

Deploy preview for gatsbygram ready!

Built with commit ce1de9b

https://deploy-preview-5583--gatsbygram.netlify.com

@gatsbybot
Copy link
Collaborator

Deploy preview for using-drupal ready!

Built with commit ce1de9b

https://deploy-preview-5583--using-drupal.netlify.com

@Undistraction
Copy link
Contributor Author

Undistraction commented May 30, 2018

FWIW I posted about this on the Gatsby Spectrum when I was trying to track down the problem and it seems a number of other people are dealing with the same issue.

@Undistraction Undistraction changed the title [gatsby] Fix 404 page rendered with missing props and without correct location.pathname [gatsby] WIP Fix 404 page rendered with missing props and without correct location.pathname Jun 1, 2018
@Undistraction
Copy link
Contributor Author

This is not ready to merge.

@pieh
Copy link
Contributor

pieh commented Aug 3, 2018

Hey @Undistraction, let us know if You are still insterested in working on this? Code that You change in this PR will most likely be changed by this #6918

@Undistraction
Copy link
Contributor Author

@pieh I've been meaning to take another look at this, but just starting a new Job so short on time. Do you know if this is still an issue in v2? I'll try and take a look again soon.

@KyleAMathews KyleAMathews force-pushed the master branch 5 times, most recently from fc4ca3e to 2116fff Compare September 17, 2018 20:36
@pieh
Copy link
Contributor

pieh commented Sep 20, 2018

404 page should have uniform handling with other pages now. There seems to be issue now when visiting non existing page directly (not by gatsby-link or navigate) - react app doesn't get mounted because of thrown errors.

I'll close this PR as code base evolved since this was opened.

@pieh pieh closed this Sep 20, 2018
@Undistraction
Copy link
Contributor Author

@pieh Sure. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants