static server-only post-render lifecycle method #8
Conversation
|
@bvaughn I a curious about something. This server stuff is related to Web environment, I wouldn't have any SSR on React Native, React VR, React [Insert Renderer that is not Web here] so why would be open functions based on specific platform? I would be fine if put it on React DOM but I don't think how that would work. |
|
You're right, at the moment a server-side hook primarily impacts the I think it might be possible for other renderers, like |
| } | ||
| ``` | ||
|
|
||
| This RFC proposes to add a new lifecycle hook named `componentDidServerRender` to be called after initial server render, eg: |
There was a problem hiding this comment.
I'm not a fan of this naming but I don't have better suggestions.
It does feel odd to codify a specific environment into the core component API.
There was a problem hiding this comment.
perhaps componentDidStaticRender, to signify that there will be no more render's after this? Or componentDidFinalRender?
There was a problem hiding this comment.
perhaps
componentDidStaticRender, to signify that there will be no more render's after this? OrcomponentDidFinalRender?
There could be an additional render if setState is called. This is like the old react-router Match/Miss use case.
componentDidInitialize
I think this name is too ambiguous. (What does it mean to "initialize"? Does that happen before or after render? Does it happen on the server or the client? etc)
There was a problem hiding this comment.
No, hydration is what happens when you “revive” the markup into a tree. On the web, think mounting React app into the markup.
There was a problem hiding this comment.
Input based on Next.js:
The naming and the usage of this should depends on what we are doing here.
Is there any blocking stuff here? Then are we going to return a promise?
(So a function starting with getXXX makes sense)
I don’t like running setState inside this as it might introduce unpredictable states and loops inside the server.
Here’s our proposal:
- We hope the idea of this hook to init some data for the component in the server.
- So what happen if this component initialize on the client? How to fetch that data?
- The best idea is to have a static function which accept intial props and return a promise.
- React will resolve that promise and use it as the props for the component.
We have used something similar for Next.js top level pages and it’s a success.
If we can have this for every component, that will be amazing.
There was a problem hiding this comment.
I agree with @arunoda. setState on the server feels weird because there isn't state on the server (unless we want there to somehow be).
On the client, would you still block render as well (like Next.js does)? Could a loading component (maybe with an api like ErrorBoundary) be added?
There was a problem hiding this comment.
Anyway, it seems like the idea of this this RFC is mainly for side-effects.
If so, my ^^^ comment should go into a different RFC.
But promise support for this hook will be great.
There was a problem hiding this comment.
Thanks @arunoda!
To answer your questions- based on the current line of thinking:
Is there any blocking stuff here? Then are we going to return a promise?
No. This lifecycle would synchronous, like all others.
A few people have mentioned the idea of returning promises (on both this RFC and #6). The scope for that feels much larger though.
I agree with @arunoda. setState on the server feels weird because there isn't state on the server (unless we want there to somehow be).
state does exist on the server. It gets initialized during construction can be referenced in render. (Without this, components would be really complicated to write.) It's just that state on the server is short-lived and not responsive to user-input.
I don’t like running setState inside this as it might introduce unpredictable states and loops inside the server.
I'm not sure what about this proposal would make components unpredictable or likely to cause loops (any more so than is currently possible on the client). Do you mean elaborating?
|
|
||
| # Motivation | ||
|
|
||
| Provide a place for initialization logic with side effects to safely run on the server. |
There was a problem hiding this comment.
What kind of side effects? Can we consider a few specific ones? It's not entirely obvious when people do this and why.
There was a problem hiding this comment.
Based on use-cases I've seen mentioned:
- Logging/metrics
- Localization initialization
- Redirect routing if no path-match was found
I can add some examples to the markdown though, sure!
| Provide a place for initialization logic with side effects to safely run on the server. | ||
| Provide a place for initialization logic with side effects to safely run on the server. This includes things like: | ||
| * Logging/metrics | ||
| * Routing redirects if match was found (eg the legacy react-router `Match`/`Miss` components) |
There was a problem hiding this comment.
Can you point me to the code examples? I'm still finding it hard to understand how those components worked (I'm not sure which version is the "legacy" one). Was it through context?
There was a problem hiding this comment.
I added some examples to the markdown file FWIW. I don't know how helpful or compelling they are though 😄 I've reached out to several OSS people asking for additional use-case examples from their real-world projects though. I'll update this section as I get info.
|
|
||
| `componentDidServerRender()` is invoked immediately after a component is server rendered. It is the server equivalent to `componentDidMount()`. | ||
|
|
||
| Calling `setState()` in this method will trigger an extra rendering, but it is guaranteed to flush during the same tick. This guarantees that even though the `render()` will be called twice in this case, the user won't see the intermediate state. |
There was a problem hiding this comment.
Can you explain how this works with streaming (its current implementation)? Do we disable streaming if there’s a component with this method anywhere in the tree above? I don’t see how we could “retract” what we’ve already written to the client. Same reason we didn’t implement error boundaries.
There was a problem hiding this comment.
Good question, Dan. I'm not familiar with our streaming implementation. Sounds like maybe this just wouldn't work with streaming.
There was a problem hiding this comment.
Oh. I see. 😬 I’m fairly sure it wouldn’t then.
My understanding was that we wanted to revisit how SSR works in general eventually and restructure it around the idea of pagelets. That’s when I thought we’d look into APIs like this as well as support error boundaries. But I don’t see how it could be compatible with the current model.
There was a problem hiding this comment.
Yeah. @sebmarkbage has been thinking about this stuff recently. I'll defer to him on this question. 😄
There was a problem hiding this comment.
I think it might be premature to name the method until we’ve decided how it works (including streaming). Since doing that later might expose an inconsistency with naming.
There was a problem hiding this comment.
Maybe it’s worth looking at it in tandem with other APIs Seb proposed? Our current naming scheme by @leeb was very well thought through and internally consistent. That’s because all lifecycles were named together. I’m worried separate proposals for each will hurt this consistency and cost us the ability to redesign them to make sense together.
There was a problem hiding this comment.
Sure!
Seb specifically suggested I propose this one in tandem with #6 😄 but that may have been just to get the discussion rolling.
There was a problem hiding this comment.
Are there reasons that this needs to be called after the render method? For example it could be called before render similar to componentWillMount and still allow setState calls to update state synchronously, with a name along the lines of componentWillSerialize.
There was a problem hiding this comment.
I think the idea was to afford the use case where you let the children render (and possibly execute their own side effects) and then decide to render something else. Calling it before render makes this impossible.
That said again, the proposal is incompatible with our implementation of streaming. So it’s not clear to me if we want this part (yet?) or not.
There was a problem hiding this comment.
I think the idea was to afford the use case where you let the children render (and possibly execute their own side effects) and then decide to render something else. Calling it before render makes this impossible.
Yeah. I'm writing a (maybe kind of contrived) example now that illustrates a use case like this.
Also, the general trend of RFC #6 is to move away from the early will-* lifecycle hooks because of their added complexity in async.
|
I don't see anything currently in this proposal that addresses the fact that it seems to be moving the lifecycle call from before render to after. Have you (or someone else) done research into how And is there a way to port components that need a lifecycle call before the call to |
I don't think this is quite accurate. This proposal is to add a new method- basically a sibling to The reasons
|
|
I would not add special lifecycle method depending on something internal from React (server rendering, or any else kind of rendering) without specifying what is so special. But of course, we should follow real use cases. Naming is weird, what server render is anyway? Isn't it "first render" to fetch data? I believe Next.js solved it properly with getInitialProps. Maybe React could incorporate something from Next.js, getInitialProps 😏 |
|
@bvaughn I realized in my previous comment that I forgot to thank you for your work and diligence in responding to comments. Thanks!
Wait, now I'm really confused. When I asked about the fact that RFC 6 was proposing deprecating To be clearer, I guess my practical question is: let's say I have a currently SSR-ed component that uses It sounds like the foolproof answer is "move all the code in |
|
@steida Thanks!
The thing that's "special" in this case is that (1) a full tree render has occurred but (2) there is no client environment so things like refs can't be used to access eg browser APIs.
I've attempted to list a few real use-case examples FWIW. I'd also welcome others to add their own. @aickin You're welcome! And thanks to you and others for the helpful feedback.
I understand the confusion! Sorry! I'm actually just saying that cWM will still be around (even if it's renamed with an "unsafe" prefix) but we don't recommend using it (for reasons mentioned on #6).
Depends on the use-case. Short answer would be to just use |
Two thoughts:
|
Facebook doesn't really use React's server-side rendering so there's not a lot of internal research I can do. I have looked at some popular open source libs and I've also reached out (via this RFC and via Twitter) to their maintainers asking for input.
This is a new method so no codemod is necessary (or really possible). |
|
After chatting (offline) for a while with @sebmarkbage about this, I'm going to narrow the scope of this RFC in the following ways:
I'll upload an updated copy of it in the morning. In the meanwhile, maybe we can put the discussion on pause to avoid confusion. My apologies for the trashing! |
Right, but in combo with RFC 6, you intend to remove functionality. I should have said that this RFC plus RFC 6 would be un-codemod-able. Or maybe more precisely that RFC 6 is un-codemod-able on its own because of SSR concerns, and that this RFC doesn't fix that problem. Is that fair? |
| @@ -30,10 +31,14 @@ This RFC proposes to add a new lifecycle hook named `componentDidServerRender` t | |||
| class Example extends React.Component { | |||
| componentDidServerRender() { | |||
There was a problem hiding this comment.
Looks like this is going to become a static method (look for an RFC update tomorrow) so the name will likely change to not begin with the words "componentWill" (because I think that might be confusing given the instance method names).
|
I've updated the RFC and examples. |
|
An “is” prefix tells me it’s a predicate - ie, it returns a boolean. Why not keep with the |
|
@victoriafrench initialize implies it happens just once, willMount happens though each time after unmount. |
|
@kof good point, I didn't think of that. Still, there has got to be a word that fits this and feels more self-explanatory. |
|
Thinking about it I kinda like the thought of naming it *willSerialize or something similar. It focuses on what will happen next, and not what happend on the way there. One question though - the application of this seems to be mostly analytics? Are the any other significant real world uses (now that the this and setState are gone)? /edit: typos |
|
Regarding the proposed
That's correct. I'm not really considering other use-cases currently. |
|
On the server, |
|
@ljharb We intentionally want to direct traffic away from "about to render" (server or otherwise) for reasons outlined on RFC #6. This proposal originally suggested an instance method to be run "just after server-rendering" but this was changed to be a static method because it would not have supported |
|
@bvaughn the use of a static method is pretty good. (prevents exposing public APIs) static server-only post-render lifecycle method ^^^ is not useful for any SSR app. In SSR, it's about data fetching. I don't know how to fetch data from a remote service from this API. I'm sorry to say, but this RFC doesn't add any value to React in the context of SSR If we need to support a SSR here's my suggestion
This is just the start, there are many questions we need to address if we are interested in something like above. |
|
@arunoda This RFC is not designed to make data fetching better so it’s no surprise that it doesn’t. We’re aware that async data fetching during server render is a pain point for some people but it’s not what we’re trying to solve in this thread so let’s try to stay on topic. It’s OK if you don’t have the problem that this addition solves — just don’t use this feature! If you want to submit an RFC about getInitialProps then feel free and we can discuss it there. |
|
Sure'll open a new RFC.
just don’t use this feature. I know this is going to introduce in the favor of the remove of And this is to be used in the Server. If we introduce a new API, it should add value over the existing. |
|
We’re not removing componentWillMount, but the semantics will change slightly in the future, so it can’t have side effects. This RFC is for a new method used for SSR that would be an alternative for the use cases when people are currently logging in componentWillMount on the server. Since componentWillMount will no longer be suitable for this use case. It does add value. |
|
The lifecycle proposed by this RFC is for a very specific purpose (mentioned in the RFC examples section). It's possible that it may also be useful for some other edge-case things done by component libraries. (I don't know.) Part of the reason for posting the RFC is to see if others can think of additional ways it could be used. The primary reason I've outlined though isn't really a common one, and it doesn't sound like it's one you need or use, which is okay. 😄 What you're describing sounds much more complex- (although also probably useful to way more people than this small RFC). So by all means, please feel free to start a new discussion around blocking/async server rendering support in a new RFC! 👍 PS. Sophie is right! We're not currently planning to remove |
|
@bvaughn @sophiebits got it. |
|
What would be the drawback of making the proposed callback fire even in the client-rendering case? It seems easy enough in userspace to make things only run when doing server-side rendering. I don't even mean doing As such, regardless of the specific semantics of the new hook in timing, static-ness, &c., it still seems like it would be cleaner to make the execution of the hook unconditional, and let determination of SSR-ness happen in userspace. |
|
As a strawman, consider something like a On the server, this would be called exactly once, after the initial render. On the client, this would be called after every render – that is, before each invocation of Obviously this is a bit problematic for reasons of redundancy, and out of the requirement to block A bit tangential here, but it'd also clean up user code that needs to reconcile non-React DOM things, e.g. all the cases where you just call into the same DOM manipulation functionality from both cDM and cDU. |
|
I thought I'd add some anecdotal feedback for how SSR-apps use componentWillMount currently. Based on my experience (and a quick search through 5 medium-sized apps I work with) it is mostly used in erroneous ways, for things it shouldn't (like dispatching actions and other side-effects that RFC 6 tries to prevent). I saw no valid use-cases, at least that could not (and will!) be refactored. This means I approve both of marking it unsafe but also that for us this RFC does not currently solve a use-case. I definitely don't mind the RFC and I can see how logging could be one such use-case though. |
|
That being said, I do think @taion brings up some valid points. If I did have a use-case for doing something after render that did not involve things like dispatching an action, doing something with the DOM etc, there is a good chance I would want to do it both on the server and on the client. The current proposal would entail triggering that both in cDM and in instanceMounted and while not a big deal that seems unnecessarily confusing. Being naive about the implementation and future plans my only question about this proposal is this: Will it in any way hinder/make harder the future development/refactoring of the SSR-functionality or React in general? If that is the case, it would be nice to see some more real world use-cases that this solves before deciding if adding the complexity is worth it. :) |
|
Just another datapoint: we'll likely need this lifecycle hook to switch to the new lifecycles in styled-components, otherwise we can't make server-side rendering work safely. Ref styled-components/styled-components#1664 |
|
Cool! Thanks for sharing another use case Max 😄 |
|
Thank you for your pull request. We require contributors to sign our Contributor License Agreement, and yours has expired. Before we can review or merge your code, we need you to email cla@fb.com with your details so we can update your status. |
|
Seems like the interest for this has waned and there have been no new use cases. I'm taking the liberty of closing it until we hear about the need again with more concrete examples. |
Provide a place for initialization logic with side effects to safely run on the server.
This RFC relates to the proposal to deprecate
componentWillMount(RFC #6) and has been previously discussed on GitHub.Please leave comments below for high-level topics. For feedback on specific parts of this proposal, please leave inline comments (on the markdown file).
View proposal with formatting