-
Notifications
You must be signed in to change notification settings - Fork 27.3k
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why |
@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? |
@sergiodxa @frol Yeah This layout feature is just a draft demo that can be discussed independently. The mainly idea in this PR is |
@timneutkens Could you review/merge this? |
53db6d5
to
8649e43
Compare
@arunoda @timneutkens Is there something wrong with this PR? Is it blocked by anything? |
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 🙏 |
@timneutkens Thanks for looking into this issue, and When we're releasing v5? |
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 |
Will the v5 beta already contains the renderPage API? |
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. |
Has anyone tried this with solution with Redux? Running into an issue when trying to wrap // 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
} |
@juciccio git repo doesn't have pre-compiled @RustyDev Elaborate more on "SSR isn't working quite right." As far as I can see, you expect |
@frol Using next-redux-wrapper, it's possible to have the store passed into 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. |
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. |
@RustyDev RustyDev/next-with-redux-applayout#1 - in this PR I made it working and this proves that |
@timneutkens @arunoda ping |
@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. |
@rashidul0405 Is "should be" related to correctness, performance or what? |
(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 |
Hello, v5 has been released is there any updates on this? |
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? |
@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. |
It's time to move on now 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. |
@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. |
It's on my list for Next v6. Sorry for taking longer, like I said before:
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 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. |
@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 :) |
I love this PR. It's a total must have! |
3d6601f
to
4cc09fe
Compare
Closing in favor of #4129 |
This PR implement the
renderPage
approach in Next.js core, according to discussing in #3461 (comment). & #3471This 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:
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:
and the output page view will be:
If this approach will be accepted, I think we can make this module to be a npm package that naming "next-layouts".