search

gatsbyjs / gatsby

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

[v2] Guidelines around layout changes #6127

Closed jonbellah closed 6 years ago

jonbellah commented 6 years ago

Summary

It's possible that I may be misinterpreting the documentation, but it appears that the suggested method of wrapping pages with a layout component causes those pages to re-render completely upon navigation.

Relevant information

The docs for layout in v2 say:

3. Import and wrap pages with layout component
Adhering to normal React composition model, you import the layout component and use it to wrap the content of the page.

src/pages/index.js

import React from "react"
import Layout from "../components/layout"

export default () => (
  <Layout>
    <div>Hello World</div>
  </Layout>
)
Repeat for every page and template that needs this layout.

See: https://next.gatsbyjs.org/docs/migrating-from-v1-to-v2/#update-layout-component

I've followed the guide, wrapping each of the pages in my /pages/ directory with my new Layout component. I have a little slide down animation on my header on initial page load... following the new <Layout /> component guidelines causes that to animate every time the page changes.

I understand I can use the html.js file or render routes directly from react-router inside my<Layout />, but I figured I'd surface the issue I found with the docs (or my misunderstanding of the docs, so that I can help bring a bit more clarity for others following the guide soon).

gatsby

The code for this can be found here: https://github.com/jonbellah/jonbellah.com/tree/v2

Relevant pieces of code:

Please let me know if I can provide any other clarity. I'm interested in contributing back to the docs, if this proves to be something worth addressing there.

viktorbengtsson commented 6 years ago

I'm having this exact problem. I would be interested in how to solve it.

Edit: So this is only a problem in gatsby develop. Works fine in the built site. See https://github.com/gatsbyjs/gatsby/issues/5213#issuecomment-399583628

jonbellah commented 6 years ago

I'm seeing the same issue with the built version: https://5b310bfdc6aed603a12b4cc7--jonbellah.netlify.com/contact/

timurc commented 6 years ago

Hey! I also got the same problem. I got into gatsby when it was reaaaally fresh and love it. Now, I am in the process of migrating our website from 0.x and thought "yay I'll just skip v1 and go straight to v2!"

However, I do need components that don't re-render on page transition for my new Ideas. I don't see yet how this is supposed to work. To demonstrate, here is a very barebones start of the new site:

live site: http://gatsby-transition-issue.volligohne.de/ code: https://github.com/timurc/vo-website-2/tree/gatsby-transition-issue

The heading always flashes red when re-rendered with a css transition. The are console.log()s in the layout component when it gets constructed, mounts and unmounts you can check out in the console. Surprisingly, it does not re-render when you use the next and previos buttons. I guess because the markdown based pages share a template.

For me there is no difference between development mode and the build version.

Even if it does not get fixed very soon, I would be very happy if anyone got an idea how to hack around it for now so I dont have to start this new project on v1. I would love to have some kind of root or master template where all the children live in. I am really curious how any kind of page transition is supposed to work now.

Thanks!

jquense commented 6 years ago

I think the way to avoid this is to use replaceComponentRenderer api to add your layout component above the Page component so that page switches don't unmount the Layout.

The issue here, is that the new hierarchy is Page -> Layout vs the old Layout -> Page and considering every Page component is generally different from the last, a navigate means LastPage !== NextPage which causes react to unmount the old page and mount the new one, The problem of course means that everything underneath Page also unmounts (as you'd expect).

m-allanson commented 6 years ago

Yeah I think using replaceComponentRenderer is the best way to handle this at the moment. It doesn't seem particularly intuitive to me, I wonder if there's a friendlier way we can do this?

jquense commented 6 years ago

I still think adding a layout plugin that uses that api is the best migration path. It wouldn't handle nested layouts but should be a nice inbetween solution

jonbellah commented 6 years ago

I'd be interested in taking a stab at a layout plugin. I don't have any experience with replaceComponentRenderer, but I'll dig through the docs and source this evening and come back with questions if I get stuck.

jonbellah commented 6 years ago

Okay, so I did a bit of reading through the source and thinking on an approach.

I'm thinking plugins: ['gatsby-plugin-layout'] will import the src/layouts/index.js file, since most people migrating to v2 will already be using that file (though they should still follow the upgrade guide and remove the props.children() function in favor of props.children, etc).

Optionally, a different path could be specified:

{
    resolve: 'gatsby-plugin-layout',
    options: {
        path: 'src/components/layout.js'
    }
}

Then using the replaceComponentRenderer API, wrap all pages with the specified layout component.

Limitations that I can see with this approach would be nested layouts, as mentioned, as well as lacking support for multiple layouts.

Thoughts on this approach?

timurc commented 6 years ago

Hey everyone, thanks for the hints and ideas and discussion!

I don't quite understand yet how replaceComponentRenderer can help me with my case. What I want is somehow a component that survives a page transition without being re-rendered, so that I can keep state there, have css animations running, so do stuff kinda independent of page transitions.

I modified the given example in replaceComponentRenderer again to make the heading flash red on transition and log out in the console each time the Transition component and the Layout component are constructed. It does fade the content, but that's what I need—I would love to have a component that survives the whole time and holds state.

demo: http://gatsby-transition-issue-2.volligohne.de/ code: https://github.com/timurc/gatsby-transition-issue-2

Maybe I am mistaken but is it maybe also not cool for performance if always everything is re-rendered? Isn't part of the idea of react to only re-render what's needed?

So yes @jonbellah it seems to me such a layout plugin would be really good to have. Apart from that, if this is possible with the current v2 I would be very thankful about hints on how to do it :-)

jonbellah commented 6 years ago

@timurc I may be missing it, but I don't think you're actually hooking into the replaceComponentRenderer API in that demo. I think the docs may be a little misleading, as it looks like the replaceComponentRenderer code was removed from that repo sometime after this commit.

I've been working off the example there, along with the tests I found in packages/gatsby/src/bootstrap/__tests__/resolve-module-exports.js and the PageRenderer component in packages/gatsby/src/cache-dir/page-renderer.js.

I'm still working on a proof of concept... running into some rough edges here and there, but working through them.

jonbellah commented 6 years ago

Okay, I'm a little stuck.

I have the replaceComponentRenderer stuff working... in v1. (Demo / Code)

But in v2, I'm getting an error -- TypeError: (0 , _apiRunnerBrowser.apiRunner) is not a function. I dug through the code and issue history and @jquense it looks like you may have some experience with this from https://github.com/gatsbyjs/gatsby/pull/5600... any direction you can point me in there is helpful. (Demo / Code)

KyleAMathews commented 6 years ago

gatsby-plugin-layout sounds like the best approach. And apologize for thinking about this issue when I did the remove-layout RFC :-\ Ironically, animating layout menu items is the original reason I added the layout concept to v1 😜

One layout component could be sufficient since it could switch between layouts based on routes. Since it's a normal component, it can do nesting as needed as well.

KyleAMathews commented 6 years ago

https://github.com/dantman/rfcs/blob/reparenting/text/0000-reparenting.md would be nice

KyleAMathews commented 6 years ago

@m-allanson were you planning on writing gatsby-plugin-layout? I think doing something similar to gatsby-plugin-typography where it generates some code based on the options would work well https://github.com/gatsbyjs/gatsby/blob/master/packages/gatsby-plugin-typography/src/gatsby-node.js

KyleAMathews commented 6 years ago

Or @jonbellah are you interested in doing a PR?

KyleAMathews commented 6 years ago

I verified as well that replaceComponentRenderer works as expected. I added to a little site to my gatsby-browser.js the following:

const React = require("react");
const Layout = require("./src/components/layout").default;

exports.replaceComponentRenderer = ({ props, component }) => {
  console.log({ props, component });
  return <Layout>{React.createElement(component, props)}</Layout>;
};
jonbellah commented 6 years ago

@KyleAMathews I'm interested in putting together a PR. I'll work one up this weekend. The code you posted is super helpful. Thanks!

jonbellah commented 6 years ago

@KyleAMathews I'm a bit stumped. The replaceComponentRenderer code you posted doesn't work for me.

Tried each of the following steps to no avail.

Always the same error - TypeError: (0 , _apiRunnerBrowser.apiRunner) is not a function. Am I missing something there?

KyleAMathews commented 6 years ago

Hmm that sucks... I've never seen that error so not sure. Check the generated api-runner-browser.js file in .cache?

m-allanson commented 6 years ago

I was looking at this earlier in the week and saw the same error (or something very similar). I'll dig out some more details on Monday.

On 1 July 2018 at 21:33, Kyle Mathews notifications@github.com wrote:

Hmm that sucks... I've never seen that error so not sure. Check the generated api-runner-browser.js file in .cache?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gatsbyjs/gatsby/issues/6127#issuecomment-401631410, or mute the thread https://github.com/notifications/unsubscribe-auth/AAXTaeBoxRrD9QfOyn4aSIQob_x-WgNEks5uCTIwgaJpZM4U0-FS .

timurc commented 6 years ago

I do have the same error as @m-allanson and @jonbellah. For me it only happens when I try to import a component to gatsby-browser.js, it does not seem to be connected to replaceComponentRenderer. So my guess is that something is wrong with the loader...?

Strangely, for me the example of @KyleAMathews does not work because replaceComponentRenderer does not get component. It is part of the props though and then it works:

const React = require('react');

// fails when imported: "Uncaught TypeError: (0 , _apiRunnerBrowser.apiRunner) is not a function"
// const Layout = require('./src/components/layout').default;

exports.replaceComponentRenderer = ({ props }) => {
    return <StateTest>{React.createElement(props.pageResources.component, props)}</StateTest>;
};

class StateTest extends React.Component {
    constructor(props) {
        super(props);
        console.log('constructing...'); // Does not fire on page transition!
    }
    render() {
        const { children } = this.props;
        return <div>{children}</div>;
    }
}

So in the example above the component StateTest only renders once and survives the page transitions! Now the only mystery for me is why I cant import components there. Hope that helps :-)

jonbellah commented 6 years ago

@timurc What a find... nice work!

I'll hold off on moving anymore on gatsby-plugin-layout until a course has been charted for the import issue, but I'm still very much interested in contributing there.

m-allanson commented 6 years ago

This error seems to be triggered if the component you're importing also imports Gatsby's Link component.

Here's a demo repo: https://github.com/m-allanson/gatsby-v2-layout-test

This gatsby-browser.js will work after changing layout.js to remove the Link import.

pieh commented 6 years ago

This error seems to be triggered if the component you're importing also imports Gatsby's Link component.

So this is because from importing from gatsby which when we follow imports will cause circular import to browser apiRunner (in loader.js). I think if we inject apiRunner into loader.js instead of importing it will fix both this and https://github.com/gatsbyjs/gatsby/issues/6235

m-allanson commented 6 years ago

I've opened a PR which fixes the Uncaught TypeError: (0 , _apiRunnerBrowser.apiRunner) is not a function error.

However we might need to do some more thinking about how this should work. Using replaceComponentRenderer to implement a persistent layout works. But it only renders your layouts on the client, it doesn't SSR render them. You could use the SSR API replaceRenderer, but that doesn't play nicely with any other plugin that already uses replaceRenderer.

Maybe this calls for a couple of new APIs? something like wrapPageClient and wrapPageSSR? A gatsby-plugin-layout plugin could implement these. They could also be useful for setting up React context providers (although I've not looked into how that works with SSR).

jonbellah commented 6 years ago

I see that #6272 made it in this morning. I'll work on gatsby-plugin-layout this weekend and report back if I get stuck again.

I agree SSR definitely needs to be handled, I'll dig in on that. I think the new API's sound like a solid approach.

jonbellah commented 6 years ago

Just keeping lines of communication open: I have a PoC working, but doing some cleanup. Will have a WIP PR in the next couple days.

jonbellah commented 6 years ago

alertthesegentoopenguin-size_restricted

Still working...

jonbellah commented 6 years ago

So I have a conceptual issue that I'm chewing on and could use some thoughts on. I have gatsby-browser.js working as expected using replaceComponentRenderer (though I haven't implemented the wrapPageClient API yet).

I've been working on the SSR side (and have been bouncing off rough edges here and there), and ultimately what I've landed on is one of two approaches.

Context

At first I was attempting to use replaceRenderer and wrap everything with the layout component. The problem here is that the replaceStaticRouterComponent API hook happens just before replaceRenderer, so if you have a GatsbyLink component in your <Layout /> then you run into an error (since there's no router around your layout).

The other downside of replaceRenderer is that only one plugin can implement it (though providing an API for other plugins/devs would help gatsby-plugin-layout not be an impediment).

How things work so far

Questions

I'm leaning towards using the replaceStaticRouterComponent API, instead of replaceRenderer, then implementing the StaticRouter directly in gatsby-plugin-layout as a wrapper around a specified layout component.

The other issue I'm having with this approach is that the documentation for replaceStaticRouterComponent appears to be TBD, so I'm not sure how stable the API is (and an issue search returned little in the way of results).

Anyways... thanks for being patient as this PR takes me a bit to feel my way around the core codebase and whack-a-mole problems as I run into them.

timurc commented 6 years ago

@jonbellah Thanks for your efforts so far! I am missing the time currently to dig into the gatsby source myself at the moment and also lack the deep understanding of the inner workings there but appreciate so much you're looking into it. For my current project, it would be really needed, otherwise the whole concept of the site is kinda not working. I am now trying to find a fix until this lands in the main repo but not very successful so far.

Adding my layout to replaceComponentRenderer only works in development for me and fails in build with the following error: 

react-dom.production.min.js:188 TypeError: Cannot read property 'markdownRemark' of undefined
    at t.render (blog-post.js:11)

The template does not get the data it that contains markdownRemark. Here is the branch i tried: https://github.com/timurc/vo-website-2/tree/try-saving-state

I now went for a pragmatic way: I save the state in global window object on componentWillUnmount to restore it on componentDidMount. That feels super stupid and won't help with css animations, but I don't know how else to fix it: https://github.com/timurc/vo-website-2/commit/cb9cf8292fa1df22c728fd89b9e739575a1ba8d5

To give some context, for the kind of playful websites we build this is one huge stand-out feature of gatsby. I know I should not complain about a beta not being feature complete–I just want to elaborate why this is important to us. Transform animations and keeping state / animations running during the route change is one thing a normal static website can't do.

I keep looking forward for movement in this issues or hints on how to work around this! Thanks for your help 🙂

jonbellah commented 6 years ago

@timurc I'm still wrapping my head around much of the core codebase, particularly the v2 codebase... I admittedly didn't have a chance to spend much time digging into your specific issue, but it looks like error you're seeing is that blog-post.js isn't getting the data prop from pageQuery. Is that issue present when you remove your gatsby-browser.js file?

I saw this issue recently with a duplicate query name, as well (though the console spits out an appropriate error for that).

I ask because I'm wondering if the issue is less with your replaceComponentRenderer code and more in line with an issue related to the move towards the StaticQuery work in v2. Though that doesn't seem to be the case, I figured it's worth at least asking.

jonbellah commented 6 years ago

@m-allanson @KyleAMathews Just wanted to ping you gents to see if you have any thoughts on https://github.com/gatsbyjs/gatsby/issues/6127#issuecomment-404821665 - I've halted work on gatsby-plugin-layout until I get some feedback there.

Thanks!

m-allanson commented 6 years ago

@jonbellah I haven't had a chance to dig into your comment yet, but chatted about this issue with @KyleAMathews and @pieh yesterday. @KyleAMathews is planning to come back round to this when he can clear some time for it. I'm not sure when that will be, but wanted you to know that this is definitely on the radar. Hope that helps!

timurc commented 6 years ago

@jonbellah thanks for the hints! This week is stuffed with other things, I'll try that next week.

@m-allanson That sounds great, thanks for updating us!

jonbellah commented 6 years ago

@m-allanson Completely understand being busy... I know y'all have a ton of stuff on your plate, but I didn't @ you in the first comment so I wanted to make sure you saw. Thanks!

bw commented 6 years ago

Just jumping in here-- I don't want to be one of those people who just comments a +1, but I think that Gatsby should message in the v1-to-v2 upgrade guide that this is a major design shift (from layout wrapping pages, to pages wrapping layouts). I just spent ~3 hrs of work moving everything to v2, only to realize this flaw (lack of CSS animations is a dealbreaker for us).

I think clarity around the changed encapsulation is important. Just my 2c.

Edit: I made a PR, #6700, that messages these changes and also includes a link to this ongoing discussion.

cpboyd commented 6 years ago

@timurc @jonbellah I'm also seeing the same page query issues in builds.

I wonder if it's related to @m-allanson's #6750 (which hopefully means it's been fixed).

If not, have you found any work-arounds? Should we be using <StaticQuery> instead of page queries?

Edit: I just rolled back my graphql-request to 1.6.0 and still have the same issue. The Layout component's <StaticQuery> gets called, but not the page (which is, effectively, a child element).

I'm guessing that maybe Gatsby is treating pages like children when wrapped by a layout?

cpboyd commented 6 years ago

So, it seems the page query issue in builds is caused by the replaceComponentRenderer in gatsby-browser.

By commenting it out, page queries worked as expected.

alpgumus commented 6 years ago

Here is my solution. Let me know if you need help passing down props or anything else, I played around with this quite a lot.

gatsby-browser.js:

export const replaceRouterComponent = ({ history }) => ({ children }) => (
      <Router history={history}>
        <YourLayout>{children}</YourLayout>
      </Router>
)

gatsby-ssr.js:

export const replaceStaticRouterComponent = () => ({ children, ...props }) => (
  <StaticRouter {...props}>
    <YourLayout>{children}</YourLayout>
  </StaticRouter>
)
cpboyd commented 6 years ago

@alpgumus Interesting...

After you mentioned that, I just noticed that the <StaticRouter> is currently added before replaceRenderer is called in static-entry.js

@m-allanson @KyleAMathews Is there any reasoning why replacing the routers, as @alpgumus has done, would properly run the graphQL page queries when the following doesn't seem to do so?

export const replaceComponentRenderer = ({ props }) => {
    const children = React.createElement(props.pageResources.component, props);
    return (
        <YourLayout>
            {children}
        </YourLayout>
    );
};

I've also tried to use replaceStaticRouterComponent (without the layout component) with the replaceComponentRenderer to see if simply having it would resolve the issue, but it doesn't.

DylanVann commented 6 years ago

@alpgumus could you include imports in that code? Is it just off of gatsby (the router components I mean)? Thank you

alpgumus commented 6 years ago

@DylanVann import { StaticRouter, Route } from 'react-router-dom'

@timurc Try this in gatsby-ssr.js if you want the data props on your layout:

export const replaceStaticRouterComponent = () => ({ children, ...props }) => (
  <StaticRouter {...props}>
    <Route
      render={routeProps => {
        const pageRendered = children.props.render(routeProps)
        const {data} = pageRendered.props
        return <YourLayout {...{data}}>{pageRendered}</YourLayout>
      }}
    />
  </StaticRouter>
)
DylanVann commented 6 years ago

@alpgumus Awesome, that worked, thank you!

This is what I ended up with. Router props are passed since I needed to know what page I was on in the layout.

// gatsby-ssr.js
const React = require('react')
import { StaticRouter, Route } from 'react-router-dom'
const Layout = require('./src/components/Layout').default

export const replaceStaticRouterComponent = () => ({ children, ...props }) => (
    <StaticRouter {...props}>
        <Route
            render={routeProps => {
                const pageRendered = children.props.render(routeProps)
                const { data } = pageRendered.props
                return (
                    <Layout {...{ data }} {...routeProps}>
                        {pageRendered}
                    </Layout>
                )
            }}
        />
    </StaticRouter>
)
// gatsby-browser.js
const React = require('react')
import { Router } from 'react-router-dom'
const Layout = require('./src/components/Layout').default

export const replaceRouterComponent = ({ history }) => ({ children }) => (
    <Router history={history}>
        <Layout history={history}>{children}</Layout>
    </Router>
)
ken0x0a commented 6 years ago

I love how gatsby, manage data, handle all necessary staff without pain.

But without Layout I need to write same code in all page.

I tried @alpgumus way, but somehow page become so heavy and chrome freeze, react-headroom create 

<div class="headroom-wrapper" style="height: 124px;">

(sometime 125px)

even I passed disableInlineStyles option...

If not need one Layout, just can do like this.

const Layout = ({ children }) => (
  <>
    {children}
  </>
)

and also, it was easy switch many layout from layout component using pathname.

Come back my lovely layout!!! plz

ken0x0a commented 6 years ago

it works only first loaded page. If move to next page, props don't change. heeeeeeeelp me!

import React from 'react'

import { Layout } from '@src/components/Layouts'

export const replaceComponentRenderer = ({ props, loader }) => {
  console.log({ props, loader })
  const Component = props.pageResources.component
  return (
    <Layout location={props.location}>
      {React.createElement(Component, props)}
    </Layout>
  )
}
octalmage commented 6 years ago

Any clue why Chrome froze? That might be helpful for figuring out if this can be a real solution.

Did React Headroom create a bunch of extra dom elements? Does it happen without React Headroom?

ken0x0a commented 6 years ago

Thank you for trying to help @octalmage !

React Headroom didn't create any extra dom element, just create style="height: ...px".

The code bellow is working as expected. I hope it helps someone like me :D

class ReplaceComponent extends React.Component {
  render() {
    const Component = this.props.pageResources.component

    return (
      <Layout location={this.props.location}>
        <Component {...this.props} {...this.props.pageResources.json} />
      </Layout>
    )
  }
}

export const replaceComponentRenderer = ({ props, loader }) => {
  console.log({ props, loader })
  return React.createElement(ReplaceComponent, { ...props, loader })
}

strange, now this also work...

export const replaceComponentRenderer = ({ props, loader }) => {
  console.log({ props, loader })
  const Component = props.pageResources.component

  return (
    <Layout location={props.location}>
      {React.createElement(Component, {
        ...props,
        // ...props.pageResources.json,
      })}
    </Layout>
  )
}

I'm sorry for create a lot use less text here. but worth to try if have same problem 👍

cpboyd commented 6 years ago

After going over @ken0x0a's posts and back over page-renderer.js everything seems to be a little clearer.

I do worry that we might not be taking the proper pathContext: (Although, maybe this doesn't matter since it's been deprecated... Maybe it's just there for legacy compatibility?)

    const pathContext =
      process.env.NODE_ENV !== `production`
        ? this.props.pageContext
        : this.state.pageResources.json.pageContext

    const props = {
      ...this.props,
      ...this.state.pageResources.json,
      pathContext,
    }

As a result, I'm now using:

export const replaceComponentRenderer = ({ props, loader }) => {
    const {pageResources, ...origProps} = props;
    const pathContext =
      process.env.NODE_ENV !== `production`
        ? props.pageContext
        : pageResources.json.pageContext;

    const newProps = {
        ...origProps,
        ...pageResources.json,
        pathContext
    };

    return (
        <Layout location={props.location}>
            {React.createElement(pageResources.component, newProps)}
        </Layout>
    );
};

Since I'm also using material-ui and redux, I've also created an OuterRoot component and use this in gatsby-browser.js:

export const wrapRootComponent = ({ Root }, options) => () => {
  return (
    <OuterRoot>
      <Root />
    </OuterRoot>
  )
}

And this in gatsby-ssr.js: Note: There's no need to re-use the Layout component since replaceComponentRenderer is called by the page-renderer in the build process

export const replaceRenderer = ({
  bodyComponent,
  replaceBodyHTMLString,
  setHeadComponents,
}) => {
    // Get the context of the page to collected side effects.
    const muiContext = getPageContext();

    const bodyHTML = renderToString(
        <JssProvider
          registry={muiContext.sheetsRegistry}
          generateClassName={muiContext.generateClassName}
        >
          <OuterRoot muiContext={muiContext}>
            {bodyComponent}
          </OuterRoot>
        </JssProvider>
    );

    replaceBodyHTMLString(bodyHTML);
    setHeadComponents([
      <style
        type="text/css"
        id="server-side-jss"
        key="server-side-jss"
        dangerouslySetInnerHTML={{ __html: muiContext.sheetsRegistry.toString() }}
      />,
    ]);
}

Thanks for all the input!

nopjia commented 6 years ago

Hi all. I've been following this thread and just want to get some clarity on this issue of "layouts re-render completely upon navigation." Is this something that is going to be addressed for v2 release, or are we users expected to add the above code snippet to address this issue ourselves?

octalmage commented 6 years ago

There’s talks of a plugin to handle it for you, but the final solution is still being developed.