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

renderPage API for top-level component rendering & layout feature implementation #3552

Closed
wants to merge 2 commits into from

Conversation

tz5514
Copy link

@tz5514 tz5514 commented Jan 9, 2018

This PR implement the renderPage approach in Next.js core, according to discussing in #3461 (comment). & #3471

This way can make core API of Next.js stay bare minimal, yet flexible.
With this core API, we can define a static property "renderPage" of page component.
It will be used to decide how page render at top level.

ex:

class IndexPage extends React.Component {
  static renderPage({ Component, props, url }) {
    return (
      <AppLayout>
        <Component {...props} url={url} />
      </AppLayout>
    )
  }

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

And this is enough for us to implement any extra features for persistent layout outside of Next.js core.

So I also implement a sample of third-party extension for layout feature in this PR (outside of Next.js core).

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 '../layouts/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'
})

and the output page view will be:
image

If this approach will be accepted, I think we can make this module to be a npm package that naming "next-layouts".

Copy link
Contributor

@frol frol left a comment

Choose a reason for hiding this comment

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

This should close #88! LGTM!

@tz5514 Thank you!

@sergiodxa
Copy link
Contributor

Why static layout and static parentLayout instead of just static layout and compose them N levels? So page X can have LayoutA and LayoutA can have a LayoutB applied and they can just be composed.

@frol
Copy link
Contributor

frol commented Jan 10, 2018

@sergiodxa The example demonstrates that given this small change in the core, it is possible to implement quite an advanced third-party API.

Should this advanced example be extracted into a separate PR?

@tz5514
Copy link
Author

tz5514 commented Jan 11, 2018

@sergiodxa @frol Yeah This layout feature is just a draft demo that can be discussed independently. The mainly idea in this PR is renderPage API of Next.js core.
If you guys feel confusing, I think I can move the example to a separate PR 😃 .

@frol
Copy link
Contributor

frol commented Jan 12, 2018

@timneutkens Could you review/merge this?

@tz5514 tz5514 force-pushed the custom-render-page-function branch from 53db6d5 to 8649e43 Compare January 12, 2018 15:25
@frol
Copy link
Contributor

frol commented Jan 22, 2018

@arunoda @timneutkens Is there something wrong with this PR? Is it blocked by anything?

@timneutkens
Copy link
Member

We'll discuss/review this PR after v5 is released. Since it's a rather big semantic change and we've been discussing it internally too. Thanks for your patience 🙏

@Nishchit14
Copy link

@timneutkens Thanks for looking into this issue, and When we're releasing v5?

@frol
Copy link
Contributor

frol commented Jan 26, 2018

Since it's a rather big semantic change

I just want to point out that this PR does not change the existing behavior, it adds new API. While I personally believe that this new API should be used in 99% of the Next.js projects, it is not a breaking change and it is still a valid React JS code, so if you decide on a better default API (which will be a breaking change), it shouldn't be an issue to migrate from this renderPage API to a new one.

@imagine10255
Copy link

Will the v5 beta already contains the renderPage API?

@juciccio
Copy link

Guys I have a problem when I try to install this PR. The npm install fails when it tries to build the next.js module.

@RustyDev
Copy link

RustyDev commented Feb 13, 2018

Has anyone tried this with solution with Redux? Running into an issue when trying to wrap AppLayout using withRedux (next-redux-wrapper) so the provider is top-level. SSR isn't working quite right. Just want to make sure that this has been tested in that scenario.

// AppLayout

export default withRedux(configureStore, mapStateToProps, mapDispatchToProps)(
  AppLayout
);

// reducer

export function fetchPlaylist(playlist, url) {
  return dispatch => {
    return fetch(url)
      .then(response => response.json())
      .then(
        response => {
          const { result, entities } = normalize(response.tracks, [
            trackSchema,
          ]);
          dispatch(
            fetchPlaylistSuccess( playlist, response.title, response.meta, result, entities )
          );
        }
      );
  };
}

// Page

Popular.getInitialProps = async function({ store, isServer, pathname, query }) {

  // ...

  const initData = await store.dispatch(fetchPlaylist(playlist, url));

  return initData
}

@frol
Copy link
Contributor

frol commented Feb 13, 2018

@juciccio git repo doesn't have pre-compiled dist folder, so you cannot install Next.js directly from the repo with npm install <repo>. Follow the guide from the contributing.md.

@RustyDev Elaborate more on "SSR isn't working quite right." As far as I can see, you expect store to be passed to getInitialProps, but I am not sure if that has ever been the case.

@RustyDev
Copy link

RustyDev commented Feb 13, 2018

@frol Using next-redux-wrapper, it's possible to have the store passed into getInitialProps via its HOC. When trying to use SSR and Redux using AppLayout (instead of on every page), I'm able to log and see the dispatch happen initially with SSR in my terminal, but it looks like the store is possibly being re-instantiated afterwards and resets back to the initial state. SSR debugging is challenging so it's hard to tell where the bug is. It could very well be my code. Hoping someone else can double check when I add a repo to test.

I was hoping to have a solution with Redux where I can wrap the app using AppLayout and then have the store be accessible in the top-level layout so that the page, header and footer all have access. The reason I was excited about this pull request is that in my app, the footer holds an audio player and is persistent between pages.

<App>
  <Provider>
    <AppLayout>
      <HeaderContainer/>
      {this.props.children}
      <FooterContainer/>
    </AppLayout>
  </Provider>
</App>

The promise of this pull request is that we can have a single layout wrapper and it seems logical to then want to wrap it with the the redux provider. I feel like I won't be the only one who will want this ability and see this as the solution.

@RustyDev
Copy link

RustyDev commented Feb 13, 2018

Here's an example repo. This has Next v5 with this PR applied, taken from this fork.

It's deployed here where you can see the store reset to 00:00:00 when loading.

This is the intended result.

Again, I hope this is considered relevant to this PR because this solution makes it easy to add persistent components (header or footer) to next apps. It would be great to also be able to wrap all of them in a Redux store since every other withRedux example wraps each page individually and wouldn't take into account the persistent containers. I'm hoping that there's something in my configuration that's not correct and that this will work eventually.

@frol
Copy link
Contributor

frol commented Feb 15, 2018

@RustyDev RustyDev/next-with-redux-applayout#1 - in this PR I made it working and this proves that renderPage API plays just fine with Redux (and, probably, the rest of the React ecosystem). It also shows that the magic of next-layout and next-redux-wrapper needs to be extended to handle renderPage API correctly. To be honest, I am not a fan of the magical solutions so the plain renderPage with explicit Redux integration as it was in the demos before next-redux-wrapper work much better for me.

@frol
Copy link
Contributor

frol commented Feb 22, 2018

@timneutkens @arunoda ping

@rashidul0405 rashidul0405 mentioned this pull request Feb 26, 2018
1 task
@rashidul0405
Copy link
Contributor

rashidul0405 commented Feb 26, 2018

@frol RustyDev/next-with-redux-applayout#1 looks like an odd solution, global store should be connected to top-level component instead of every page.

@frol
Copy link
Contributor

frol commented Feb 26, 2018

@rashidul0405 Is "should be" related to correctness, performance or what? next-redux-wrapper is designed to wrap pages (it stores the global store in its own "global" variable, which is then attached to pages, so it doesn't re-create the store). You may want to play with _document.js API if you want to have something truly global.

@frol
Copy link
Contributor

frol commented Mar 9, 2018

@timneutkens

We'll discuss/review this PR after v5 is released.

(c) #3552 (comment)

What are the particular issues with this PR? Do you have a better idea on how to tackle #88? Can you unlock people who need this issue to be resolved?

/cc @arunoda

@ghost
Copy link

ghost commented Mar 19, 2018

Hello, v5 has been released is there any updates on this?
my current solution is far from pretty and I'd love to hear some better solutions.

@krazyjakee
Copy link

What does this addition give that this example does not? https://github.com/zeit/next.js/tree/canary/examples/layout-component

Am I able to maintain the state of a layout over multiple routes without re-mounting it?

@isBatak
Copy link

isBatak commented Mar 21, 2018

@krazyjakee if you rewrite layout component like this https://gist.github.com/isBatak/9ee6ca051ed3ea3435af31b2c7c40d2f you will see that it is actually re-mounting on each page change.

@Nishchit14
Copy link

It's time to move on now
This issue is the heart of the Nextjs, but still, people are focusing on v5 rather than solving the real problem.

Next is too good, we can develop small to medium size of the application, but for the enterprise level application, It is still risky.

Thank you, Guys.

@tz5514
Copy link
Author

tz5514 commented Mar 22, 2018

@Nishchit14 I can't agree with you more. Honestly, I'm also disappointed that they keep silent on this issue. I think the solution in this PR is stand by lots of people, but they still adhere to discussing this problem in internal, and didn't give it a priority.

Next.js is awesome, and one of the most important project in React ecosystem. We really thanks for core team's work. But this issue really stagnate too long.

@timneutkens
Copy link
Member

It's on my list for Next v6. Sorry for taking longer, like I said before:

Since it's a rather big semantic change and we've been discussing it internally too. Thanks for your patience 🙏

To give some context:

I'm basically focusing on making 5.x very stable before moving on to introducing big new things (like this one) 👍 5.1 is coming very soon 👍, and after that I can focus on multiple things, including this. I'm very aware of how many people want this feature.

It's time to move on now
This issue is the heart of the Nextjs, but still, people are focusing on v5 rather than solving the real problem.

It's a twofold, if we gave you this feature instead of working on improvement of v5 you, or someone else, would go ahead and say: my hot reloading state is broken. Or whatever other issues there were.

@Nishchit14
Copy link

@timneutkens thank you for the breaking silence, feeling better now 😃

@tz5514 I agree with you sir, I respect the core team and very thankful to them, But due to long silence on this issue I was feeling like a frozen in Iceland. It's never been easier to migrate to the new framework and when you stuck in between then It is more painful.

Thank you team again :)

@glemiere
Copy link

glemiere commented Apr 4, 2018

I love this PR. It's a total must have!
@tz5514 is there a way we can start working on your idea of a next-layouts package until it's merged ?
@timneutkens when do you plan to release the version 6 ?

@tz5514 tz5514 force-pushed the custom-render-page-function branch from 3d6601f to 4cc09fe Compare April 5, 2018 23:07
@timneutkens
Copy link
Member

Closing in favor of #4129

@timneutkens timneutkens closed this Apr 9, 2018
@timneutkens timneutkens mentioned this pull request Apr 10, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Apr 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.

None yet