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

Allow pages to be render functions (allows persistent layout with no magic) #3461

Closed
wants to merge 2 commits into from

Conversation

jdeal
Copy link

@jdeal jdeal commented Dec 16, 2017

By adding the ability to define a page as a function instead of a component, you can easily create persistent layouts by the natural way that React diffs the tree.

The proposal here is that if the exported function is named render, then it's treated as a render function rather than a component. In that case, the return value of that function is rendered rather than creating an element from the component. Any matching root components stay mounted across page transitions. Because you're in control of what is rendered at the root, you can have multiple possible roots that mount/unmount as desired. For example, you could have pages a and b use LayoutFoo and c and d use LayoutBar. LayoutFoo would stay mounted while on a and b, but it would be unmounted when going to c and d. In this way, this PR is more generic than #3288.

Only a slight tweak to lib/app.js is required to make this possible. The rest of this PR is a new example to demonstrate a persistent layout with render function pages.

@frol
Copy link
Contributor

frol commented Dec 16, 2017

Nice and reasonable workaround! @jdeal Thank you!

@arunoda
Copy link
Contributor

arunoda commented Dec 18, 2017

@jdeal wow this is pretty neat.
But, now we offer two APIs to render the page. Which is not good.

I can only take this PR, if this is the only the way.
The default export of the top level pages directory is a function(no special naming like render), but not a React component.

We can make this backward compatible and warn users to follow this approach.


But before we do that, we need to know how @rauchg @nkzawa @sergiodxa and @timneutkens thinks about this.

@rauchg
Copy link
Member

rauchg commented Dec 18, 2017

This is super interesting @jdeal. Thanks for sharing. Are there any downsides to always applying this new behavior without checking for the name?

@jdeal
Copy link
Author

jdeal commented Dec 18, 2017

Thanks @arunoda and @rauchg for taking a look!

I can happily switch to always treating plain functions as render functions and detecting React classes for backward compatibility.

The one downside I can imagine would be if someone is currently depending on that behavior. In other words, if they have a stateless function component at the top and then some stateful components underneath, those child components are currently unmounting/remounting. If that's actually desirable in their specific case, or if they've done other workarounds to "fix" that behavior, then moving to a render function would suddenly surprise them and possibly break their app.

@arunoda
Copy link
Contributor

arunoda commented Dec 18, 2017

@jdeal

The one downside I can imagine would be if someone is currently depending on that behavior

This is a good point.
That's why introducing this as a breaking change is something we should consider.

@@ -83,7 +83,11 @@ class Container extends Component {
// https://github.com/gaearon/react-hot-loader/issues/442
return (
<AppContainer warnings={false} errorReporter={ErrorDebug}>
<Component {...props} url={url} />
{
Component.name === 'render'
Copy link
Contributor

@arunoda arunoda Dec 18, 2017

Choose a reason for hiding this comment

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

On other idea to wrap the Component inside a function if it's a React component class.

And do not rely on the name.

@sergiodxa
Copy link
Contributor

🤔 I'm not sure, this is not idiomatic React code, most people don't know you can just call a React stateless component as simple functions passing props as arguments neither expect it.

So we should add warning and teach people on how this works. And as @arunoda said we're going to have a new API to render a page, this means more documentation needed and more possible errors (specially due people don't expect that to work).

@corysimmons
Copy link
Contributor

Seeing as how this is such a desired feature (and has been for a while), I think adding a bit of documentation for this functionality is well worth it. If something more idiomatic comes along, you can just release a new major version and refer people to the changelog. :\

@sergiodxa
Copy link
Contributor

sergiodxa commented Dec 18, 2017

@rauchg what about #3471 ? is not a breaking change, and React components will be still be used as usual.

BTW I don't care about documenting more Next.js features, my concern is with this we need to document another way React components can be used, it's not a Next.js feature, is an undocumented way to use React.

@arunoda
Copy link
Contributor

arunoda commented Dec 19, 2017

@sergiodxa If I were designing the Next.js from scratch. I'd go with this option.
(exporting a function directly)

Like this:

const Page = (props) => (
  <Layout>
    ...
  </Layout>
)

Page.getInitialProps = () => ({
   name: 'Hello'
})

export default Page

And we don't support exporting React classes.

But now, it's pretty hard to do this change since we've made the pages API super stable.
So, it's pretty hard to do a breaking change.

That's why I like #3471.

@jdeal
Copy link
Author

jdeal commented Dec 19, 2017

@arunoda @sergiodxa I had thought of a similar approach to #3471, but I went with the render function instead because it's more flexible. What if you want your Layout component to provide a render callback instead of passing it a child component? What if you want to pass different props to your Layout component from different pages? What if you don't want a Layout component at all, but instead you want to make some kind of dynamic determination of what to render? Having the <Layout><Component .../></Layout> structure enforced places rigid requirements on your Layout component rather than allowing standard React flexibility to do what makes sense for you app.

An important side effect of this is that the static Layout component approach simply pushes the problem further down the tree. The page component (Component) will still get unmounted/remounted with every page change. In some cases this may be desirable, but in other cases it may not. The render function approach makes it easy to layer various components which stay mounted as desired. The static Layout component approach forces a separation between the parts that stay mounted and the parts that don't.

What about deprecating the current behavior in v5 with a warning and allowing opt-in in a config to the new behavior? Then v6 could switch to the new behavior as a default.

Some other options for providing a render function without the special render naming...

// Provide a Page class that creates a wrapped render function.
import Page from 'next/page'

export class MyPage extends Page {
  render() {
    return <Layout>...</Layout>;
  }
}
// Provide a createPage helper that wraps a render function.
import createPage from 'next/page'

export createPage(() => <Layout>...</Layout>);

Neither of these is substantially different from the render naming, but they are more explicit so it's clear to at least go look for next/page in the docs and see what's special.

You could also compromise with something like this:

export default class AboutPage extends React.Component {
  static renderPage = ({Component, ...props}) => <Layout><Component {...props}/></Layout>
  render () {
    return (
      <div>
        about
      </div>
    )
  }
}

This way, technically, you keep the same top-level API, but there's an escape hatch that lets take over rendering. Effectively, that means you could do this:

export default class AboutPage extends React.Component {
  static renderPage = () => <Layout><div>about</div></Layout>
}

That's fine, but maybe just a little weird in the sense that we're just hijacking the React.Component class to hold a render function.

@pozylon
Copy link

pozylon commented Dec 19, 2017

I like the idea of "createPage" because that way getInitialProps can go together with a pure render function:

// Provide a createPage helper that wraps a render function.
import createPage from 'next/page'

export createPage({
    getInitialProps: (context) => {
        // ...
    }
}, () => <Layout>...</Layout>);

@frol
Copy link
Contributor

frol commented Dec 19, 2017

If I can put my two cents again, I completely agree with:

An important side effect of this is that the static Layout component approach simply pushes the problem further down the tree.

So, I think #3471 is not a complete solution while the approach described by @jdeal makes things flexible. The exact API (either a "render" function, or "renderPage", or "Page" class) needs more thought, indeed.

I don't like the magic "render" function naming just because it doesn't say much to those who just inspect the code without reading Next.js documentation. I would prefer a more explicit way of declaring your intent.

"Page" class and "createPage" expands the API surface, and also make things confusing given two sets of APIs (React.Component/stateless component [native React way] and Page/createPage [Next.js way]) are expected.

The compromise of static renderPage method function (the naming doesn't really matter), which is similar to #3471, yet gives more control over the layout, looks the best option to me. It is flexible, optional, not a breaking change, and not magical.

@frol
Copy link
Contributor

frol commented Jan 9, 2018

Let's close this in favor of #3552

@apapacy
Copy link

apapacy commented Jan 20, 2018

Why this persitence not work for me?
image
image

@jdeal
Copy link
Author

jdeal commented Jan 20, 2018

@apapacy Probably because you need to npm link the example to this version. If you install in the example directory, it will pick up latest which doesn't have this change. I'm guessing this PR will be closed though and #3552 might take its place, so you might give that one a try instead.

@apapacy
Copy link

apapacy commented Jan 20, 2018

I'm clone your fork canary branch.
image
node v8.9.4
node v 7.10.0
node v6.12.0

This example does not work as expected. I'm do not understand how it may work. Is a new layout created each time? In #3552 +export default applyLayout(IndexPage) makes some magic.

@jdeal
Copy link
Author

jdeal commented Jan 20, 2018

@apapacy Here it is working for me:

From the examples/with-render-function-pages folder, you need to npm install && npm link ../.. though. Otherwise, it's going to pick up your globally installed next.

It will not create a new Layout each time because the render naming causes the pages to be considered render functions instead of stateless function components.

@apapacy
Copy link

apapacy commented Jan 20, 2018

Yes. It work. You update lib/app.js. I'm not detect this fact.

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

Closing in favor of #4129

@timneutkens timneutkens closed this Apr 9, 2018
@PetrSnobelt PetrSnobelt mentioned this pull request Apr 11, 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.

9 participants