gatsbyjs / gatsby

The best React-based framework with performance, scalability and security built in.
https://www.gatsbyjs.com
MIT License
55.27k stars 10.31k forks source link

Styled component doesn't have access to its (theme) props on 404.html page #5498

Closed markph1990 closed 6 years ago

markph1990 commented 6 years ago

Description

Components being used on 404 page don't have access to passed props. I have this issue with styled components (accessing theme prop inside them), so example will be based on that.

Steps to reproduce

gatsby new gatsby-site yarn add styled-components

layouts/index.js file:

...
import { ThemeProvider } from 'styled-components'
...
const Layout = ({ children, data }) => (
  <ThemeProvider theme={{ foo: { bar: 'baz' } }}>
    ...
  </ThemeProvider>
)
...

404.js file:

import React from 'react'
import styled from 'styled-components'

const Debugger = styled.div`
  ${({ theme }) => console.log(theme.foo.bar)};
`

const NotFoundPage = props => (
  <div>
    <h1>NOT FOUND</h1>
    <p>You just hit a route that doesn&#39;t exist... the sadness.</p>
    <Debugger />
  </div>
)

export default NotFoundPage

Expected result

Component on 404 page should have access to its props.

Actual result

Running gatsby build && gatsby serve and accessing nonexistent route yields such results: screenshot from 2018-05-21 19-01-21

Environment

File contents (if changed)

gatsby-config.js: N/A package.json: added styled-components ^3.2.6 gatsby-node.js: N/A gatsby-browser.js: N/A gatsby-ssr.js: N/A

Undistraction commented 6 years ago

I can confirm this issue. The theme is absent from the props object on the 404 page. The SSR'd version of the page is displayed briefly, followed by an error due to the missing theme object on which the page components rely.

@markph1990 Have you found a workaround?

markph1990 commented 6 years ago

For now I wrap jsx contents of NotFoundPage in ThemeProvider from styled-components, passing in theme as a prop. This issue might not be 100% correlated to gatsby but also to styled-components. I'll have to check it further.

Undistraction commented 6 years ago

@markph1990 Thanks. I'm pretty sure this is a Gatsby issue. I can actually navigate to /404 even in production and the page renders without issues, however a genuine missing page causes the error.

This error occurs locally using yarn serve.

If a live site would help with debugging, here the demo for a starter I've been working on deployed to Surge and the exact same issue is present:

If I rip out the components on the 404 page and just render the props that are passed into the page I can see the page is rendered twice.

On the initial render the props are:

{
  "match": { "path": "/", "url": "/", "params": {}, "isExact": false },
  "location": { "pathname": "/404.html", "search": "", "hash": "" },
  "history": {
    "action": "POP",
    "location": { "pathname": "/404.html", "search": "", "hash": "" }
  },
  "staticContext": {},
  "data": {
    "site": {
      "siteMetadata": {
        "structure": { "pages": { "notFound": { "title": "Page Not Found" } } }
      }
    }
  },
  "pathContext": {}
}

On the second render the props are (and theme is undefined in any styled components):

{
  "page": true,
  "location": { "pathname": "/404.html" },
  "data": {
    "site": {
      "siteMetadata": {
        "structure": { "pages": { "notFound": { "title": "Page Not Found" } } }
      }
    }
  },
  "pathContext": {}
}

You can see this happen here

You can see this locally by:

  1. Clone https://github.com/Undistraction/gatsby-skeleton-starter
  2. yarn
  3. yarn build
  4. yarn serve
  5. open http://localhost:9000/

Workaround

Wrap the contents of the 404 page in its own ThemeProvide, for exampler:

const FourOhFourPage = ({ data, location }) => (
  <ThemeProvider theme={theme}>
    <Page>
      <NotFoundMessage/>
    </Page>
  </ThemeProvider>
)
Undistraction commented 6 years ago

@KyleAMathews Can you suggest where the problem might lie? I'm happy to dig into it and try and resolve, but if you have any guidance that would be really helpful.

Is there any way to prevent Gatsby compressing JavaScript on Build. It makes this very hard to debug as it strips all logging out.

reimertz commented 6 years ago

Also just saw this in production. Thanks for the workaround @Undistraction.

ssw1ss commented 6 years ago

@Undistraction I'm pretty sure this is because Gatsby renders a default 404 page when in development mode. This is why when you route to "/404" you see the expected result, whereas when you travel to a route that simply doesn't exist, the default gatsby 404 component is rendered instead. In production mode, your 404 page will be seen instead.

Undistraction commented 6 years ago

@olive338 This issue is related to the 404 page in production, not in development. In production your own 404 page should be served when a page isn't found. Gatsby is serving a 404 page in production, however any styled components that make use of a theme do not have access to that theme. The theme is supplied using ThemeProvider which uses React's context API to make the theme available to all styled-components that are rendered in the DOM tree below the React component which was wrapped by ThemeProvider. This works correctly for all other pages, so there is something different about how Gatsby is rendering the 404 page.

To recap, the 404 page is loaded, however it will not render correctly due to the theme (that should supplied to any styled-components via their props) being undefined.

pieh commented 6 years ago

If anyone is up for doing some investigation - here's good place to start for differences between normal page and 404 in build - https://github.com/gatsbyjs/gatsby/blob/074dac30e4edd405dde5ce2f7e38dd139cb50978/packages/gatsby/cache-dir/production-app.js#L169-L184

Undistraction commented 6 years ago

@pieh I'm on it. Think I see the problem. Will get a PR in tonight.

reimertz commented 6 years ago

Seems that we just need to pass props to that createElement for it to work as intended?

On Mon, May 28, 2018 at 5:53 PM Pedr Browne notifications@github.com wrote:

@pieh https://github.com/pieh I'm on it. Think I see the problem. Will get a PR in tonight.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/gatsbyjs/gatsby/issues/5498#issuecomment-392559975, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmKh9LWp_wSS_-ti3WPbwJ_8yYEO5vZks5t3B1cgaJpZM4UHV3Q .

-- Best Regards, Piérre Reimertz reimertz.co | twitter http://twitter.com/reimertz | github http://github.com/reimertz

Undistraction commented 6 years ago

@reimertz Yes. At present the props that would have been supplied to the page are not being passed on, so the only prop being passed in to the 404 page are:

{
  location: { pathname: `/404.html` }
}

I'm working on a PR that fixes this and will also make the URL of the missing page available to the 404 page which will also solve https://github.com/gatsbyjs/gatsby/issues/5544

reimertz commented 6 years ago

Aw yeh! 🙌 On Mon, 28 May 2018 at 18:20, Pedr Browne notifications@github.com wrote:

@reimertz https://github.com/reimertz Yes. At present the props that would have been supplied to the page are not being passed on, so the only prop being passed in to the 404 page are:

{ location: { pathname: /404.html } }

I'm working on a PR that fixes this and will also make the URL of the missing page available to the 404 page which will also solve #5544 https://github.com/gatsbyjs/gatsby/issues/5544

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/gatsbyjs/gatsby/issues/5498#issuecomment-392564852, or mute the thread https://github.com/notifications/unsubscribe-auth/AAmKh5Nbsv63bSd9ZzZvyDIhQePAeF6nks5t3CPfgaJpZM4UHV3Q .

-- Best Regards, Piérre Reimertz reimertz.co | twitter http://twitter.com/reimertz | github http://github.com/reimertz

Undistraction commented 6 years ago

@reimertz @markph1990 If you get a chance please can you pull https://github.com/gatsbyjs/gatsby/pull/5583 and see if it fixes this issue for you. Please make sure you delete the .cache dir before you rebuild.

martynhoyer commented 6 years ago

Just popping in to say I've got the same issue, and the same thing is happening with react-intl's <IntlProvider> too.

I'll try that PR branch shortly.

Undistraction commented 6 years ago

@martynhoyer Thanks. That would be really helpful.

martynhoyer commented 6 years ago

@Undistraction ok silly question... what's the best way to run your PR version in my build? I tried npm link with a clone of your repo without much success, then tried adding "gatsby": "git+https://github.com/Undistraction/gatsby.git#wip-fix-404-page-props", into my dependencies, but it won't install. Is there a more standard way?

Undistraction commented 6 years ago

@martynhoyer Not a stupid question at all. It's a little convoluted because Gatsby uses a monorepo. Take a look at the contributing docs here: https://www.gatsbyjs.org/docs/how-to-contribute/, only pull my branch rather than creating your own topic branch.

martynhoyer commented 6 years ago

Ok I think I've got that working, but I've still got the problem with the 404 page.

It could definitely be me doing something wrong though. I checked my project's node_modules/gatsby/cache-dir/production-app.js and it looks like it has the changes from your PR in there though, so I'm assuming I'm using the right code. Is there any other way to confirm I've got it set up correctly?

What I did was:

  1. Clone your gatsby repo (https://github.com/Undistraction/gatsby.git) and checkout the wip-fix-404-page-props branch.
  2. Install gatsby-dev-cli and from my project, set the path to your cloned repo.
  3. Run gatsby-dev from my project root.
  4. Run gatsby-build and gatsby-serve from my project root.

Is that right?

Undistraction commented 6 years ago

Did you run npm run watch, otherwise I don't think it will pick up the changed files.

If you can't get that working you could always just copy the changes (only 1 change in 1 file) to the file in node_modules/gatsby.

Either way, make sure you nuke Gatsby's cache before running.

martynhoyer commented 6 years ago

Ok, yeah, I tried both, and still no joy. Deleted caches in both my project and in the node_modules/gatsby just to make sure.

Loading 404.html directly is still fine.

martynhoyer commented 6 years ago

Just had another go after successfully using gatsby-dev in another PR, so I'm pretty sure I'm doing that correctly, but it's still not working.

Is it working for you @Undistraction ?

It'd be ace to get someone else to test it. @markph1990 @reimertz are you able to have a look in your environments?

markph1990 commented 6 years ago

For me also Undistraction's PR doesn't seem to fix the issue (tried it several times, with/without npm run watch, cleared cache several times). I set everything up as per gatsby docs for contributing (cloning repo, gatsby-dev-cli, etc).

Undistraction commented 6 years ago

@markph1990 @martynhoyer Looks like it isn't fully solving the problem. Been digging into this again. Are you guys using the theme object to store functions by any chance? There seems to be something going on where functions are stripped off the theme object and only primitive values are making it through.

As in:

const theme = {
  example: () => {}
}

If my theme looks like this:

{
  alpha: 1,
  bravo: 2,
  charlie: () => {}
}

The theme that my styled-component receives will look like:

{
  alpha: 1,
  bravo: 2,
}

This is only true for styled components used within the 404 page component.

martynhoyer commented 6 years ago

My theme doesn't have any functions in it - it's just:

{
  palette: {
    noir: '#2c393e',
    violet: '#5d5dff',
    bleu: '#18aded',
    rose: '#ff57b6',
    blanc: '#fff',
    grisLight: '#8f959b',
  },
  status: {
    danger: '#ff2ba3',
    success: '#1ec076',
  },
  social: {
    facebook: '#6075f4',
    linkedin: '#0073b1',
    twitter: '#3edfff',
    instagram: '#e4228c',
  },
  shadows: {
    default: '0.175rem 0.175rem 0.35rem 0 rgba(0, 0, 0, 0.1);',
  },
}

My project is also failing to get the intl prop from the <IntlProvider> from react-intl if that helps at all. I don't think the problem is limited to styled-components, but seemingly to all HOCs in the layout component.

Undistraction commented 6 years ago

@martynhoyer This is a real head-scratcher. Have you tried debugging with Chrome React Plugin? Can you see the context for the components?

martynhoyer commented 6 years ago

I'm starting to think this might be a problem with the my combination of styled-components and react-intl... The tests are very inconsistent between gatsby-develop, gatsby-build/serve and then running the same build on Netlify. On Netlify styled-components seems to be fine (still broken when I serve a build locally), but it breaks when I try to put any intl stuff in there. Very very strange and needs more testing I think. I'll get back to you!

Undistraction commented 6 years ago

I spent a good chunk of time trying to work it out last night. There is something strange going on with the way the context is passed down on the 404 page. When a React Component is provided with a context (such as for SC's theme and probably in react-intl), React doesn't pass this value via props, but sets props on the component. If you use the Chrome React plugin you can check to see if the contexual values are set.

I can clearly see that the context values are being set on my FourOhFourPage component (which is not a styled component), however the theme doesn't seem to be available to any styled components within it.

Undistraction commented 6 years ago

I just tried gatsby-plugin-react-next to see if using the new Context API might work, but no dice.

marcammann commented 6 years ago

I ran into the same issue, but because we're trying to have a dynamic 404 page. I think what's happening is that the page resources don't get created for the correct path (since it doesn't really exist). Accessing the 404 page directly works because the correct page resources get loaded (however that works internally).

I ended up doing a redirect in production-app.js:

if (loader.getPage(props.location.pathname)) {
  return createElement(ComponentRenderer, {
    page: true,
    ...props,
  })
} else {
  window.location.pathname = '/404.html'
  return
  // return createElement(ComponentRenderer, {
  //   page: true,
  //   location: { pathname: `/404.html` },
  // })
}

Not very elegant, but I'm certain it has something to do with the pathname going into the rendering of the entire stack doesn't exist.

pieh commented 6 years ago

This is fixed in gatsby version 2.

jmcdl commented 3 years ago

This is still an issue for me. I could only resolve it by using @Undistraction's workaround.