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

[v2] Guidelines around layout changes #6127

Closed
jonbellah opened this issue Jun 23, 2018 · 66 comments
Closed

[v2] Guidelines around layout changes #6127

jonbellah opened this issue Jun 23, 2018 · 66 comments
Assignees
Labels
type: question or discussion Issue discussing or asking a question about Gatsby

Comments

@jonbellah
Copy link

jonbellah commented Jun 23, 2018

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
Copy link

viktorbengtsson commented Jun 25, 2018

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 #5213 (comment)

@jonbellah
Copy link
Author

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

@timurc
Copy link
Contributor

timurc commented Jun 27, 2018

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
Copy link
Contributor

jquense commented Jun 27, 2018

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
Copy link
Contributor

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?

@m-allanson m-allanson added the type: question or discussion Issue discussing or asking a question about Gatsby label Jun 27, 2018
@jquense
Copy link
Contributor

jquense commented Jun 27, 2018

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

timurc commented Jun 28, 2018

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
Copy link
Author

jonbellah commented Jun 28, 2018

@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
Copy link
Author

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 #5600... any direction you can point me in there is helpful. (Demo / Code)

@m-allanson m-allanson self-assigned this Jun 29, 2018
@KyleAMathews
Copy link
Contributor

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
Copy link
Contributor

@KyleAMathews
Copy link
Contributor

@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
Copy link
Contributor

Or @jonbellah are you interested in doing a PR?

@KyleAMathews
Copy link
Contributor

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
Copy link
Author

@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
Copy link
Author

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

  • Uninstalled my global gatsby
  • Installed -g gatsby@next
  • Cleared my .cache dir
  • Deleted my node_modules folder and reinstalled
  • Tried node 8.11.3 and 10.5.0
  • Tried children() and children in the layout itself
  • Tried with two different code bases (one from scratch)
  • Tried gatsby develop and gatsby build

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

@KyleAMathews
Copy link
Contributor

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

@m-allanson
Copy link
Contributor

m-allanson commented Jul 1, 2018 via email

@timurc
Copy link
Contributor

timurc commented Jul 2, 2018

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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Contributor

pieh commented Jul 2, 2018

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 #6235

@m-allanson
Copy link
Contributor

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

@m-allanson m-allanson added this to To Do - v2 in Gatsby v2 Release via automation Jul 3, 2018
@octalmage
Copy link
Contributor

@HriBB I've used fs.appendFileSync to debug SSR. That plus util.inspect(myObject, false, null) to get the full object output printed. There might be a better way but this works.

@DylanVann
Copy link

DylanVann commented Aug 8, 2018

Trying to use the latest Gatsby beta. I can't figure out a way to get a template component working with the available APIs.

Closest I've got:

// gatsby-ssr.js
import React from 'react'
import Layout from './src/components/Layout'
import { Location } from '@reach/router'
import { renderToString } from "react-dom/server"

export const replaceRenderer = ({
                               bodyComponent,
                               replaceBodyHTMLString,
                           }) => {
    const newBody = (
        <Location>
            {props => (
                <Layout {...props}>
                    {bodyComponent}
                </Layout>
            )}
        </Location>
    )
    const newBodyString = renderToString(newBody)
    replaceBodyHTMLString(newBodyString)
}
// gatsby-browser.js
import React from 'react'
import Layout from './src/components/Layout'
import { Location } from '@reach/router'

export const wrapRootComponent = ({ Root }) => {
    const WrappedRootComponent = props => (
        <Location>
            {routeProps => (
                <Layout {...routeProps}>
                    <Root {...props} />
                </Layout>
            )}
        </Location>
    )

    return WrappedRootComponent
}

The problem with this is that it breaks the emotion plugin. If wrapRootComponent was available in gatsby-ssr.js that would be great.

@pieh
Copy link
Contributor

pieh commented Aug 8, 2018

@DylanVann I'm working on solution for this. It will involve some new APIs that will allow wrapping page component in both ssr and browser and without limitation to just single use (like with your example + using emotion plugin)

@pieh
Copy link
Contributor

pieh commented Aug 8, 2018

Solution here will be few parts:

  • implement new APIs mentioned in comment above - this plus updating plugins to use new APIs will allow painless use of multiple plugins that currently are conflicting (like we currently really only support single wrapRootComponent or replaceRenderer/replaceBodyHTMLString)
  • creating gatsby-plugin-layout that will use new APIs - this will be very simple plugin that just will skip you from writing some boilerplate in both gatsby-ssr and gatsby-browser for most common use case + adding documentation how to handle data flow from layout to page and page to layout (using react context)

@alpgumus
Copy link

alpgumus commented Aug 9, 2018

@DylanVann try this interim, let me know if you have more questions

// gatsby-browser.js
const LayoutRenderer = ({ PageComponent, pageProps }) => 
  <YourLayout {...pageProps}><PageComponent {...pageProps} /></YourLayout>

export const replaceComponentRenderer = ({ props: { pageResources, ...pageProps } }) => 
  <LayoutRenderer PageComponent={pageResources.component} pageProps={pageProps} />
// gatsby-ssr.js
export const replaceRenderer = ({ bodyComponent }) => {
  const page = bodyComponent.children.props.children
  bodyComponent.children.props.children = <YourLayout {...page.props}>{page}</YourLayout>
  return bodyComponent
}

@DylanVann
Copy link

DylanVann commented Aug 9, 2018

@alpgumus That code almost works. I'm hitting: Duplicate replaceRenderer found, skipping gatsby-ssr.js for plugin: gatsby-plugin-emotion.

So it looks like I'd have to implement emotion SSR myself using this API.

@pieh That sounds awesome, looking forward to trying it out.

I think for now I'll just not upgrade to the latest beta. I'll revisit in a few weeks.

@cpboyd
Copy link
Contributor

cpboyd commented Aug 9, 2018

@pieh I'm glad to see that someone's working on allowing for multiple iterations of replaceRenderer.

It's not necessarily true that someone who needs one for css-in-jss stuff like gatsby-plugin-emotion or gatsby-plugin-jss (or material-ui like myself) won't also need another renderer for something else.

@pieh
Copy link
Contributor

pieh commented Aug 9, 2018

Actually it will probably be separate API and not replaceRenderer - problem with replaceRenderer is replaceBodyHTMLString function - once this is called to wrap stuff, other plugins can't do anything insteresting, because at this point we work with html string and not react components. So this is problematic only because we don't have better way of injecting providers right now.

I did some tests with jss plugin with my new APIs and it worked nicely, but will probably need to change those because they are hard to explain (you know your API is not well designed if you can't figure out how to document it so people understand it :) )

@pieh
Copy link
Contributor

pieh commented Aug 9, 2018

This most likely be copy of wrapRootComponent from gatsby-browser to gatsby-ssr (to handle providers) and new wrapPageComponent (in both gatsby-browser and gatsby-ssr to target this layout problem (as well as possibly use it for page transitions).

Additional change would be ability to chain results from one plugin as input for next plugins, so this isn't limited to single plugin - so you could wrap root component with provider for jss and then wrap that in provider for redux and wrap that in provider for react context etc

@pieh
Copy link
Contributor

pieh commented Aug 10, 2018

So first PR is opened - #7204, after that this will follow - pieh@7442f70 (gatsby-plugin-layout)

and a lot of doc changes

@pieh
Copy link
Contributor

pieh commented Aug 20, 2018

Migration guide is updated to mention gatsby-plugin-layout for usecases that require layout component to stay mounted between page changes - https://next.gatsbyjs.org/docs/migrating-from-v1-to-v2/#remove-or-refactor-layout-components, so I'll close this issue for now.

Next item on to-do list is to put this information somewhere else in documentation, so people can discover if they are not migrating from v1 and need this behaviour.

@dallanlee
Copy link

Is there a solution for nested layouts?

@jessepinho
Copy link
Contributor

jessepinho commented Sep 5, 2019

Same question as @dallanlee.

My situation: I have a <Layout /> component that lives at the top of the React tree. I use wrapPageElement to put it on the page, and that works fine.

One of my page types (created via createPage()) is an article page. I use an <ArticlePage /> component for this page. When clicking a link from one article to another article in my Gatsby site, the entire <ArticlePage /> component and its children are re-mounted, rather than simply being re-rendered with new props.

Possible solution

I could move <ArticlePage /> into wrapPageElement. The downside is that all pages would include the code for <ArticlePage />, including non-article pages. This results in more bytes of JavaScript being sent to the browser, and the browser having to parse more code.

To avoid the extra JS payload, I could make ArticlePage an asynchronously loaded component — i.e., use the import() function and thus only load the component if it's rendered. The problem there is that it won't actually be rendered as part of Gatsby's SSR. Maybe I could use an async import in gatsby-browser.js's wrapPageElement and a non-async import in gatsby-ssr's wrapPageElement?

Even if I do, this feels like a suboptimal solution. Among other reasons, pulling the <ArticlePage /> component up to wrapPageElement means I can no longer include a static page query for it with arguments, which is needed for the page.

The "proper" solution, it seems, would be to simply rerender (rather than re-mount) the page component when the path changes, as long as the React component is the same between paths.

Thoughts?

@cyberwombat
Copy link

@jessepinho I had a similar issue whereas my front page has a slightly different layout than the rest. I was able to use the createPage hook for this as outlined in the multiple layout section here. I am not actually clear on what happens internally as far as whether my front page layout is completely rerendered or not but for my use case its enough.

@jessepinho
Copy link
Contributor

@cyberwombat the issue is that the code for both layouts is always loaded, in that case — which means a lot of extra bytes of JS sent to the browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: question or discussion Issue discussing or asking a question about Gatsby
Projects
No open projects
Development

No branches or pull requests