-
Notifications
You must be signed in to change notification settings - Fork 561
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
static server-only post-render lifecycle method #8
Changes from 2 commits
909e547
fe1c7bf
8951bf3
bb1accd
3d0a659
9b323a8
e6c4bf1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
- Start Date: 2017-12-11 | ||
- RFC PR: (leave this empty) | ||
- React Issue: (leave this empty) | ||
|
||
# Summary | ||
|
||
Provide a server-side rendering equivalent for `componentDidMount`. | ||
|
||
This RFC relates to the proposal to deprecate `componentWillMount` ([RFC #6](https://github.com/reactjs/rfcs/pull/6)) and has been [previously discussed on GitHub](https://github.com/facebook/react/issues/7671). | ||
|
||
# Basic example | ||
|
||
Because `componentWillMount` is the only lifecycle method currently invoked during server rendering, it is sometimes used for things like logging, eg: | ||
```js | ||
class Example extends React.Component { | ||
componentWillMount() { | ||
if (typeof window === 'undefined') { | ||
// Server-side | ||
} | ||
} | ||
|
||
componentDidMount() { | ||
// Client-side | ||
} | ||
} | ||
``` | ||
|
||
This RFC proposes to add a new lifecycle hook named `componentDidServerRender` to be called after initial server render, eg: | ||
```js | ||
class Example extends React.Component { | ||
componentDidServerRender() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). |
||
// Server-side | ||
} | ||
|
||
componentDidMount() { | ||
// Client-side | ||
} | ||
} | ||
``` | ||
|
||
# Motivation | ||
|
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
* Initializaing shared localization data | ||
|
||
Typically, `componentWillMount` is used for this, but [RFC #6](https://github.com/reactjs/rfcs/pull/6) proposes to remove this method. (It is not a very ergonomic solution anyway.) The logic could be moved to the component constructor but it's generally considered bad practice for a constructor to have side effects. | ||
|
||
# Detailed design | ||
|
||
## `componentDidServerRender()` | ||
|
||
`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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. @sebmarkbage has been thinking about this stuff recently. I'll defer to him on this question. 😄 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there reasons that this needs to be called after the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
|
||
Use this pattern with caution because it can impact performance. However, it is sometimes necessary for cases like localization or routing where you need to override the results of the initial render based on new information. | ||
|
||
# Drawbacks | ||
|
||
This proposal is backwards compatible. | ||
|
||
# Alternatives | ||
|
||
The alternative to this new lifecycle hook is to continue to feature test (eg `typeof window`) inside of either `componentWillMount` or the class constructor. | ||
|
||
# Adoption strategy | ||
|
||
Release a minor update to version 16 with support for the new lifecycle hook. | ||
|
||
Pending the outcome of [RFC #6](https://github.com/reactjs/rfcs/pull/6), include messaging about this new hook in the `componentWillMount` deprecation warning. | ||
|
||
Coordinate with popular libraries (eg [react-router](https://reacttraining.com/react-router/), [react-intl](https://github.com/yahoo/react-intl)) to ensure this new hook meets their needs before `componentWillMount` is deprecated. | ||
|
||
# How we teach this | ||
|
||
Write a blog post for [reactjs.org](https://reactjs.org/) explaining the motivation for this new hook as well as showing a basic example of its usage. Add it to the [component reference API](https://reactjs.org/docs/react-component.html) as well. | ||
|
||
# Unresolved questions | ||
|
||
None? |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
perhaps
componentDidStaticRender
, to signify that there will be no more render's after this? OrcomponentDidFinalRender
?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.
componentDidInitialize
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.
There could be an additional render if
setState
is called. This is like the old react-routerMatch
/Miss
use case.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
applicationDidRender
?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @arunoda!
To answer your questions- based on the current line of thinking:
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.
state
does exist on the server. It gets initialized during construction can be referenced inrender
. (Without this, components would be really complicated to write.) It's just thatstate
on the server is short-lived and not responsive to user-input.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?