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

Persistent layout (top-level app component) supporting page by page without any workaround #3471

Closed
wants to merge 2 commits into from

Conversation

tz5514
Copy link

@tz5514 tz5514 commented Dec 18, 2017

This PR give a simple approach to support persistent layout and setting it page by page without any workaround.

Just use the static property to define the layout component in a single page. It will be something like:

// pages/about.js
import AppLayout from '../components/AppLayout'

export default class AboutPage extends React.Component {
  static pageLayout = AppLayout

  render () {
    return (
      <div>
        about
      </div>
    )
  }
}

and

// components/AppLayout.js
export default class AppLayout extends React.Component {
  render () {
    return (
      <div>
        <h1>App header</h1>
        {this.props.children}
      </div>
    )
  }
}

If a page's layout was undefined, it will works just like original situation and won't break anything.
So you can specify the layout setting page by page, instead of a global layout.
I think this approach can resolve persistent layout problem without any pain that won't harm original page mechanism.

@sergiodxa
Copy link
Contributor

sergiodxa commented Dec 18, 2017

This can be a good idea, and it provides a way to set a different layout per page instead of a global one.

@tz5514 tz5514 changed the title Persistent layout supporting page by page without any workaround Persistent layout (top-level app component) supporting page by page without any workaround Dec 18, 2017
@arunoda
Copy link
Contributor

arunoda commented Dec 19, 2017

This is also something nice.

lib/app.js Outdated
@@ -72,9 +72,17 @@ class Container extends Component {

render () {
const { Component, props, url } = this.props
const Layout = Component.pageLayout || null
const component = Layout ? (
<Layout>
Copy link

Choose a reason for hiding this comment

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

@tz5514 What about passing the props and url to Layout? That way, the Layout component could make dynamic rendering decisions if needed. Could also pass Component in case it subsequently wanted to pass different props to the page component. That way, I think you'd have full control over rendering.

Copy link
Author

@tz5514 tz5514 Dec 19, 2017

Choose a reason for hiding this comment

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

That's a good idea. The layout probably has its own props (from hoc or other),
so I think we can pass the page props by independent "pageProps". like this:

<Layout pageProps={props} url={url}>
   <Component {...props} url={url} />
 </Layout>

Copy link
Member

Choose a reason for hiding this comment

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

@tz5514 i'd just spread the props from the page. The layout can't have any other props than these considering that you expose the layout as a static property 👌 A HOC might expose props on the layout. But passing them off using {...props} seems like the logical way to do it. Since then we can just say: A layout component receives the same props as a top level page. Arunoda, rauchg and me are still discussing if we want getInitialProps on layout components btw. I'm thinking of exposing it as a community-made HOC, since you might want getInitialProps to run at the same time as the page component's getInitialProps, or run them in sync.

Copy link
Author

@tz5514 tz5514 Dec 19, 2017

Choose a reason for hiding this comment

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

@timneutkens Yes! I actually want to put getInitialProps on layout components too.
Because there are some async data actions being shared for different pages, like: SSR token auth. It will be so convenience if we put getInitialProps on layout components.
BTW, I think run page and layout's getInitialProps in sync is more intuitional(layout' getInitialProps first, page's getInitialProps later).

So if we do this, it will be "a layout's props is basically decided by layout's getInitialProps" rather than "a layout component receives the same props as a top level page".
How do you guys think? thanks.

// pages/about.js
import AppLayout from '../components/AppLayout'

export default class AboutPage extends React.Component {
  static pageLayout = AppLayout
  static getInitialProps() {
    return { foo: 100 }
  }
 
  render () {
    // { url, foo }
    console.log(this.props);

    return (
      <div>
        about
      </div>
    )
  }
}

// components/AppLayout.js
export default class AppLayout extends React.Component {
  static getInitialProps() {
    return { bar: 200 }
  }

  render () {
    /* { bar, url, pageProps: { foo } }
       or 
       { bar, url, foo } 
       when about page
    */
    console.log(this.props); 

    return (
      <div>
        <h1>App header</h1>
        {this.props.children}
      </div>
    )
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so here's what I'd like to do. Implement getInitialProps for the layout component. Then, if some wants to run them at the same time they can use this HOC I've drafted and change the awaits to a Promise.all.

const layoutGetInitialProps = (ComposedComponent) => {
  const ComponentLayoutComponent = Component.layout
  const Layout = ({layoutProps}) => <ComponentLayoutComponent {...layoutProps} />

  return class extends React.Component {
    static async getInitialProps(ctx) {
      let layoutProps = {}
      let props = {}
      if(typeof ComponentLayoutComponent.getInitialProps === 'function') {
        layoutProps = await ComponentLayoutComponent.getInitialProps(ctx)
      }

      if(typeof ComposedComponent.getInitialProps === 'function') {
        props = await ComposedComponent.getInitialProps(ctx)        
      }

      return {layoutProps, ...props}
    }

    static layout = Layout

    render() {
      const props = {...this.props}
      delete props.layoutProps
      return <ComposedComponent {...props} />
    }
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

Also, this HOC proves the static layout property is just as flexible as the render function.

import AppLayout from '../components/AppLayout'

export default class IndexPage extends React.Component {
static pageLayout = AppLayout
Copy link
Member

Choose a reason for hiding this comment

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

Lets make this layout instead of pageLayout. Both are clear as to what they do, and layout is easier to remember 😄 👍

Copy link
Author

Choose a reason for hiding this comment

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

ok! thanks for advice!

@frol
Copy link
Contributor

frol commented Dec 19, 2017

It is interesting that after having #88 for over a year, two competing PRs popped up almost at the same time. May I invite you to participate in the comments to #3461 (comment) ?

@tz5514 tz5514 force-pushed the persistent-layout-page-by-page branch from 05346de to b689947 Compare December 19, 2017 13:24
@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

@frol of course! I'm agree with your comment and I'm thinking about how to improve the layout flexibility in my approach.

@timneutkens
Copy link
Member

@tz5514 also feel free to join https://zeit.chat 👍

@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

@timneutkens @frol Do you guys think multiple layer design for layout is a good idea?
We can defined it some like this:

// components/AppLayout.js
export default class AppLayout extends React.Component {
  render () {
    return (
      <div>
        <h1>App Header</h1>
        {this.props.children}
      </div>
    )
  }
}
// components/ContentLayout.js
import AppLayout from './AppLayout'

export default class ContentLayout extends React.Component {
  static parentLayout = AppLayout

  render () {
    return (
      <div>
        <hr/>
        {this.props.children}
        <hr/>   
      </div>
    )
  }
}
// pages/about.js
import ContentLayout from '../component/ContentLayout'

export default class AboutPage extends React.Component {
  static layout = ContentLayout

  render() {
    return (
      <p>about</p>
    )
  }
}

and the output of about page will be :
image

the demo PR is here: https://github.com/tz5514/next.js/pull/1/files

@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

I do this because the developers usually need multiple layer for defined something shared.
If the layout mechanism only support for single layer, the developers must wrap second level layout in Top level layout. It will cause part of layout component no longer persistent in some case.

BTW, I really like "renderPage" approach in #3461 (comment)
Allowing we custom the renderPage function is really a flexible way.
But it also cause part of layout component no longer persistent in some case (If the developer don't know that can be a trap).

For example:

// pages/foo.js
export default class FooPage extends React.Component {
  static renderPage = ({Component, ...props}) => (
    <TopLayout>
      <SecondLayout>
        <Component {...props}/>
      </SecondLayout>
    </TopLayout>
  )
  render () {
    return (
      <div>foo</div>
    )
  }
}
// pages/bar.js
export default class BarPage extends React.Component {
  static renderPage = ({Component, ...props}) => (
    <TopLayout>
      <p>paragraph that only show at BarPage</p>
      <SecondLayout>
        <Component {...props}/>
      </SecondLayout>
    </TopLayout>
  )
  render () {
    return (
      <div>bar</div>
    )
  }
}  

In this case, when you routing to BarPage from FooPage, the SecondLayout will be no longer persistent.

On the contrary, we can do this just like:

export default class TopLayout extends React.Component {
  render () {
    return (
      <div>
        {(this.props.url.pathname == '/about') && (
          <p>paragraph that only show at BarPage</p>
        )} 
        {this.props.children}
      </div>
    )
  }
}

Then SecondLayout can still keep persistent when we do routing to BarPage from FooPage.
So I think a little bit of constraints but allowing defined by multiple layer is more appropriate for layout mechanism, and it will be more intuitional for most of people.

@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

There is an another proposal that I think about:
We can define fully layout chain in each page, just like:

// pages/about.js
import ContentLayout from '../component/ContentLayout'

export default class AboutPage extends React.Component {
  static layouts = [AppLayout, ContentLayout]

  render() {
    return (
      <p>about</p>
    )
  }
}

and no longer define parent layout in ContentLayout:

export default class ContentLayout extends React.Component {
  render () {
    return (
      <div>
        <hr/>
        {this.props.children}
        <hr/>
      </div>
    )
  }
}

The advantage of this way is that we can replace the part of layout chain in any page without pain.
We don't need to create another Layout class to build branch of layout chain.
How do you guys think? thanks.

@frol
Copy link
Contributor

frol commented Dec 19, 2017

Allowing we custom the renderPage function is really a flexible way.
But it also cause part of layout component no longer persistent in some case (If the developer don't know that can be a trap).

Well, that is a native React reconciliation process, so this is up to the developer to keep that point in mind and that part is easily solvable by just assigning the same key to the SecondLayout. I have put a demo together:

class TopLayout extends React.Component {
  componentWillMount() {
    console.log("TopLayout will mount")
  }
  componentDidMount() {
    console.log("TopLayout did mount")
  }
  componentWillUnmount() {
    console.log("TopLayout will unmount")
  }

  render() {
    console.log("TopLayout render")
    return <div><div><span>TopLayout:</span><span>{(new Date()).toString()}</span></div><div>{this.props.children}</div></div>
  }
}

class SecondLayout extends React.Component {
  componentWillMount() {
    console.log("SecondLayout will mount")
  }
  componentDidMount() {
    console.log("SecondLayout did mount")
  }
  componentWillUnmount() {
    console.log("SecondLayout will unmount")
  }

  render() {
    console.log("SecondLayout render")
    return <div><div><span>SecondLayout:</span><span>{(new Date()).toString()}</span></div><div>{this.props.children}</div></div>
  }
}


class MyComponent extends React.Component {
  componentWillMount() {
    console.log("MyComponent will mount")
  }
  componentDidMount() {
    console.log("MyComponent did mount")
  }
  componentWillUnmount() {
    console.log("MyComponent will unmount")
  }

  render() {
    console.log("MyComponent render")
    return <div>{(new Date()).toString()}</div>
  }
}

const renderPage1 = ({Component, ...props}) => {
  return (
    <TopLayout>
      <SecondLayout key="second"><Component {...props} /></SecondLayout>
    </TopLayout>
  )
}

const renderPage2 = ({Component, ...props}) => {
  return (
    <TopLayout>
      <p>render2</p>
      <SecondLayout key="second"><Component {...props} /></SecondLayout>
    </TopLayout>
  )
}


export default class extends React.Component {
  state = {
    renderPage: renderPage1,
  }
  componentDidMount() {
    setTimeout(() => { this.setState({ renderPage: renderPage2 }) }, 5000)
  }

  render() {
    const { renderPage } = this.state
    return renderPage({Component: MyComponent})
  }
}

Once you open such a page, you will see that the components are mounted & rendered and after 5 seconds, when the renderPage1 is replaced with renderPage2, you will only see that the components were re-rendered (without remounting):

TopLayout render
SecondLayout render
MyComponent render

Without the key="second", however, you will obviously encounter the remounts:

TopLayout render
SecondLayout will mount
SecondLayout render
MyComponent will mount
MyComponent render
SecondLayout will unmount
MyComponent will unmount
MyComponent did mount
SecondLayout did mount

I think, it is devs responsibility to control React reconciliation.

@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

@frol Thank for your commenting and explaining and you're definitely right.
But I think this layout API will be used in Next.js by most of developers. So we need to design a method that most of developers won't use it by wrong way (Actually lots of React beginners might even don't know how to use "key") and keep it simple as possible.

@frol
Copy link
Contributor

frol commented Dec 19, 2017

Well, having the renderPage API you can make a HOC or a base class to bring the support of static layout property, while this is not going to work the other way around. In my opinion, given that Next.js went with bare minimal, yet flexible, API for routing enabling third-party extensions, renderPage approach should win.

@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

@frol Oh that is a great viewpoint for me. If we have renderPage means we can implement the my approach outside of Next.js core. That's is also great I think, thanks for discussing!

@tz5514
Copy link
Author

tz5514 commented Dec 19, 2017

@jdeal @timneutkens @sergiodxa @arunoda How about this demo PR: tz5514#2?

This demo only implement the renderPage approach in Next.js core, according to discussing in #3461 (comment).
Just like @frol's opinion, this way can make core API of Next.js stay bare minimal, yet flexible.
And that is enough for us to implement any extra features for layout outside of Next.js core.

So I already implement a sample of third-party extension for layout feature (Here)

It support the multiple layer, layout's getInitialProps, and getInitialProps's concurrent mode.
so we can do something like:

// layouts/AppLayout.js
import React from 'react'
import delay from '../modules/delay'

export default class AppLayout extends React.Component {
  static async getInitialProps () {
    return {
      delay: await delay(1000)
    }
  }

  render () {
    return (
      <div>
        <h1>App header</h1>
        {this.props.children}
      </div>
    )
  }
}
// layouts/ContentLayout.js
import AppLayout from './AppLayout'

export default class ContentLayout extends React.Component {
  static parentLayout = AppLayout
  static async getInitialProps () {
    return {
      delay: await delay(2000)
    }
  }

  render () {
    return (
      <div>
        <hr />
        {this.props.children}
        <hr />
      </div>
    )
  }
}
// pages/about.js
import React from 'react'
import delay from '../modules/delay'
import applyLayout from '../modules/next-layouts'
import ContentLayout from '../components/ContentLayout'

class AboutPage extends React.Component {
  static layout = ContentLayout
  static async getInitialProps () {
    return {
      delay: await delay(3000)
    }
  }

  render () {
    return (
      <p>about</p>
    )
  }
}

export default applyLayout(AboutPage, {
  getInitialPropsMode: 'concurrent'
})

If this approach is a good practice, I think we can make this module to be a npm package that naming like "next-layouts" 😄 .

@jdeal
Copy link

jdeal commented Dec 20, 2017

@tz5514 I'm (unsurprisingly) a fan of this approach. 😄

A nice side effect for me is that I'll be able to create pages like this:

export default {
  renderPage: () => <Layout>Look ma, no component!</Layout>
};

Because it's just checking for a renderPage property, I don't need a component at all and almost get my render function like I wanted with #3461.

@frol
Copy link
Contributor

frol commented Dec 21, 2017

@arunoda @rauchg @timneutkens @sergiodxa Are you comfortable with the renderPage API proposed by @jdeal and implemented by @tz5514 (tz5514#2)?

@frol
Copy link
Contributor

frol commented Jan 9, 2018

Let's close this in favor of #3552

@tz5514 tz5514 closed this Jan 9, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jan 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants