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

async-safe, static lifecycle hooks #6

Merged
merged 22 commits into from Jan 19, 2018

Conversation

@bvaughn
Collaborator

bvaughn commented Dec 8, 2017

Note for React Native users: if you see a warning pointing to this page, this was most likely due to a mistake in our release process. This warning wasn’t supposed to fire until we finish writing the documentation for it, and wasn’t supposed to link to this page. We will either publish the documentation ASAP or revert this mistake in a patch release today (7th of March). We’re sorry for the disturbance.


Replace error-prone render phase lifecycle hooks with static methods to make it easier to write async-compatible React components. Provide a clear migration path for legacy components to become async-ready.

Note that I plan to submit a related RFC soon for a new server-side lifecycle, componentDidServerRender, to offset any server-rendering functionality lost by deprecating componentWillMount.

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

@bvaughn bvaughn changed the title from Initial proposal for async-friendly, static lifecycle hooks to async-safe, static lifecycle hooks Dec 8, 2017

@abritinthebay

This comment has been minimized.

abritinthebay commented Dec 8, 2017

As the RFC stands it seems written towards solving problems for one, very specific, workflow.

Maybe that’s just the examples.

The resulting interface though seems extremely verbose & complicated comparatively to the one it replaces.

I like the goal of this though. The idea is good.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Dec 8, 2017

Thanks for the input!

As the RFC stands it seems written towards solving problems for one, very specific, workflow.

The goal is to solve all workflows/use-cases in a way that's safer by default (or maybe, in a way that has fewer potential pitfalls, or requires less advanced knowledge to use correctly). If you know of use-cases that it couldn't handle, this is a great time to discuss them!

The resulting interface though seems extremely verbose & complicated comparatively to the one it replaces.

Does the proposed API actually increase verbosity or complexity? I think it just moves things around, but maybe I'm stuck looking at it with a stale perspective.

@trueadm

This comment has been minimized.

trueadm commented Dec 8, 2017

I feel this proposal has great benefits over what we have now, but ultimately should probably be merged into an entirely new component API that doesn't involve classes at all. Furthermore, to unify with our other efforts, i.e. compilation of React components, by adding more to already complicated class component will makes things even more complicated to handle.

In my opinion we need a new component API that offers:

  • the same flexibility as before, but with some sensible constraints
  • an async-first approach to dealing with lifecycle events like outlined in this proposal
  • a compiler friendly approach to dealing with things like component folding, bytecode interpretation and dead-code elimination
@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Dec 8, 2017

Thanks Dom.

I feel this proposal has great benefits over what we have now, but ultimately should probably be merged into an entirely new component API that doesn't involve classes at all.

I'm not sure what the right answer is, but the advantage of this proposal over what you're describing is that it provides an incremental path for existing components to take advantage of async. A completely new, non-class-based API seems both harder to design and much larger effort to adopt.

That being said, I recognize that I can't appreciate the compiler difficulties presented by the class component API like you can.

This method is invoked before `render` for both the initial render and all subsequent updates. It is not called during server rendering.
The purpose of this method is to initiate asynchronous request(s) as early as possible in a component's rendering lifecycle. Such requests will not block `render`. They can be used to pre-prime a cache that is later used in `componentDidMount`/`componentDidUpdate` to trigger a state update.

This comment has been minimized.

@bvaughn

bvaughn Dec 8, 2017

Collaborator

Inspiration for this method comes from facebook/react#7671 (comment):

You would rely on an external system to keep its cache around. Like Relay or some Flux store. So when you request it again, it's available. Just like when you <link rel="prefetch" /> something you don't get a direct handle on the resource. A separate request gets the data.

@sebmarkbage

This comment has been minimized.

Collaborator

sebmarkbage commented Dec 8, 2017

The goal of this proposal is not to be the final "React.next" API. It's to provide a reasonable upgrade path for a large set of components e.g. in a codemoddable way.

The assumption is that:

  1. Whatever the next API change is will be significantly different enough that you can't just codemod your way there. E.g. leaking an instance alone would bail.
  2. It is a relative easy manual change to move to these new life-cycles.
  3. There are significantly fewer components that use these particular life-cycles than there are total components.

It is also much easier to adopt full async in a large codebase this way and a small ever growing portion being compiler compatible with the new API.

Now whether it is worth doing both steps is arguable but that's the idea.

@trueadm

This comment has been minimized.

trueadm commented Dec 8, 2017

Let me take a step back and state what I feel the ultimate goal would be for me: in the future, we don't have class components anymore and state/lifecycles should instead be handled by another mechanism that doesn't force us to break out a declarative render phase into an imperative OO class phase.

That sounds bonkers, but will likely be a future RFC from me once I have time to properly explain it. That being said, incrementally, I feel we should start adding constraints to components:

  • Seal the class instance after construction, so instance variables are no longer possible
  • Come up with a new lifecycle interface (like outlined in this PR) that replaces the current lifecycle methods
@markerikson

This comment has been minimized.

markerikson commented Dec 8, 2017

@trueadm : If instance variables are no longer possible, and lifecycle methods are going away, how would React components handle non-React interop? The standard example would be:

class MyPluginWrapper extends Component {
    componentDidMount() {
        this.$theDiv = $(this.theDiv)
        this.$theDiv.somePlugin("create");
    }

    componentDidUpdate() {
        this.$theDiv.somePlugin("update", this.props.someValue);
    }

    componentWillUnmount() {
        this.$theDiv.somePlugin("destroy");
    }

    render() {
        return <div ref={theDiv => this.theDiv = theDiv} />;
    }
}

In this use case, references to non-React values are stored as instance fields, and created/updated/destroyed in lifecycle methods.

@aickin

This comment has been minimized.

aickin commented Dec 8, 2017

Thanks for your work on this RFC!

I may just not have enough background in the async discussion (is there a place where those ideas are laid out?), but I have to say that I found this RFC very difficult to understand. A few suggestions that probably would have made it easier for me and maybe might make it easier for others:

  1. Move the motivation/description of problem to the top. The code sample is long, and was somewhat hard to grok when I had no idea what was being changed or why.
  2. I'd suggest beefing up the description of the three problems you listed in Common Problems. It sounds like some of these are programmer errors; why aren't they currently causing problems? Or are they?
  3. Consider separating the code sample out into multiple examples, ideally each connected to a Common Problem. For me, it was very difficult to keep all the things going on in those lifecycle method in my head at once.
  4. It seems like there are going to be changes to the order of lifecycle methods in an async version, and this RFC assumes the reader understands what those are. (I might be wrong here; it just seemed implied by some of the text.) If so, explaining that or linking to a clear explanation would be great!

Again, thanks for thinking through these issues and for all your work. Also, I apologize for the stuff I just have totally misunderstood. I'm excited to see async moving forward!

@trueadm

This comment has been minimized.

trueadm commented Dec 8, 2017

Note: this isn't really that relevant to this PR proposal.

@markerikson I'm glad you asked :) lately I've been using a lot of ReasonReact and the place for refs is most definitely state, which would fit nicely into my createRef proposal: facebook/react#11555.

For more information on how ReasonReact deals with refs: https://reasonml.github.io/reason-react/docs/en/react-ref.html

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Dec 8, 2017

Move the motivation/description of problem to the top.

That's useful feedback for the RFC process in general. This proposal just follows the standard template.

It seems like there are going to be changes to the order of lifecycle methods in an async version, and this RFC assumes the reader understands what those are.

I don't think I meant to imply any change in ordering. Could you clarify where you thought this?

Actually @aickin, do you mind splitting up your comments and adding them inline? Large, multi-bullet comments are hard to respond to in a single thread like this. 😄

@aickin

This comment has been minimized.

aickin commented Dec 8, 2017

I don't think I meant to imply any change in ordering. Could you clarify where you thought this?

I think it was a vague impression I got from the following ideas:

  1. The async experiment you explained showed non-idempotent function calls in functions like componentWillMount, and you mentioned it's a problem that they could get called "multiple times".
  2. Those types of non-idempotent function calls should move to the new static functions, which will solve the problem.

That implied to me that the new static functions are called in a different way than their previous instance method analogues, which made me think that there are more changes to lifecycle order/number of calls. Honestly, though, it was a very rough impression that I may have just totally misread.

@aickin, do you mind splitting up your comments and adding them inline? Large, multi-bullet comments are hard to respond to in a single thread like this.

Sure, sorry!

ETA: I moved my big list of nits out from this discussion into line comments in the document. I hope they are genuinely helpful, and please let me know if they are not!

addExternalEventListeners();
this._computeMemoizeData(nextProps);

This comment has been minimized.

@aickin

aickin Dec 8, 2017

Is nextProps defined here? I may just be missing it, but I don't see where it would be defined.

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

Whoops. This was a typo. Will fix.

this.setState({ externalData })
);
addExternalEventListeners();

This comment has been minimized.

@aickin

aickin Dec 8, 2017

Adding event listeners in componentWillMount is a bug, right? I know that it would make SSR fail (since there's no DOM), and it seems like you say it's a problem in the Common Problems section. If that's right, a comment to that effect would be super helpful!

This comment has been minimized.

@sebmarkbage

sebmarkbage Dec 9, 2017

Collaborator

Flux stores are external event listeners that don't need a DOM, but also SSR alone is not necessarily that common. Especially for long tail.

This comment has been minimized.

@aickin

aickin Dec 9, 2017

Thanks, that's helpful context, but my question here is whether or not this line is currently considered a bug. It sounds like this document is saying yes, it is, although I'm not entirely sure. If so, I think a comment would help understanding. Does that make sense?

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

Yeah. This is not a good pattern. (I listed it below as a common problem we see.) I show it here only to show where it should be done instead. I've added an inline comment to this part of the example though to make it clear that it's not a pattern we recommend. 😄

### `static prefetch(props: Props, state: State): void`
This method is invoked before `render` for both the initial render and all subsequent updates. It is not called during server rendering.

This comment has been minimized.

@aickin

aickin Dec 8, 2017

IIRC componentWillMount is called in SSR. Since componentWillMount is being deprecated and replaced with prefetch and prefetch is not called in server rendering, that means that there would be no lifecycle calls on the server any more, right? Have you looked into what use cases cWM is used for on the server?

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

This is what my note in the PR description is about. I'll be adding a related RFC shortly with componentDidServerRender based on the discussions here and here. 😄

This comment has been minimized.

@aickin

aickin Dec 9, 2017

<facepalm emoji> I just totally missed that line. Sorry!

// (Async request won't complete before render anyway.)
// If you only need to pre-fetch before mount,
// You can conditionally fetch based on state.
asyncLoadData(props.someId);

This comment has been minimized.

@aickin

aickin Dec 8, 2017

The example for prefetch was a little confusing to me, since it calls a method that elsewhere in the example returns a Promise of data, but prefetch then ignores that Promise. It makes sense if you understand that asyncLoadData caches the response for the next call, but that's not really obvious. Perhaps rename that method asyncCacheData or precacheData?

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

The name is a reference to link prefetch in the browser world, since the functionality is kind of analogous. It's good feedback though. We'll see if others find the name confusing.

Avoid introducing any side-effects, mutations, or subscriptions in this method. For those use cases, use `componentDidMount`/`componentDidUpdate` instead.
### `static deriveStateFromProps(props: Props, state: State, prevProps: Props): PartialState | null`

This comment has been minimized.

@aickin

aickin Dec 9, 2017

Is there a reason for switching from passing in nextProps in componentWillReceiveProps to using prevProps in deriveStateFromProps? It may make updating code easier to keep the same semantics and names.

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

You can name the parameter nextProps if you'd prefer 😄 It's the same thing.

2. Find and fix async bugs before they impact end-users by intentionally triggering them in a deterministic way.
3. Gain confidence that our existing products could work in async.
I believe this GK confirmed what we suspected about the legacy component API: _It has too many potential pitfalls to be safely used for async rendering._

This comment has been minimized.

@aickin

aickin Dec 9, 2017

"I believe this GK confirmed ..." was a little confusing for me as a non-Facebooker. I assume it's referring to the experiment y'all did, but took me a few beats to guess that.

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

Ah, thank you. I'll change this to use non-Facebook-specific terminology.

@markerikson

This comment has been minimized.

markerikson commented Dec 9, 2017

@trueadm : so just to clarify, the place to store anything a component needs to reference would now be in "state", even if those values aren't directly related to re-rendering? The rule of thumb so far has been "if it relates to rendering, put it in this.state, otherwise store it as this.someValue".

This method is invoked before `render` for both the initial render and all subsequent updates. It is not called during server rendering.
The purpose of this method is to initiate asynchronous request(s) as early as possible in a component's rendering lifecycle. Such requests will not block `render`. They can be used to pre-prime a cache that is later used in `componentDidMount`/`componentDidUpdate` to trigger a state update.

This comment has been minimized.

@aickin

aickin Dec 9, 2017

It may be just me, but I find the prefetch-componentDidMount two-step a bit unwieldy as a component developer; it requires me to implement some caching layer and ensure I don't end up calling the data load twice, which leads to very confusing control flow. It might be nicer to let prefetch return a Promise, which would be passed in to componentDidMount/Update, though I haven't thought that through very well.

This comment has been minimized.

@sebmarkbage

sebmarkbage Dec 9, 2017

Collaborator

That seems reasonable but the whole prefetching thing is also really a micro-optimization. It's a power-feature. You can just do it in componentDidMount too.

I think this touches on a bigger issue that it is hard to pass values between different life-cycles which needs a bigger API refactor.

That said, I think it is better that it is awkward because in an async scenario you can be interrupted before componentDidMount and then get rerendered. In which case you would get another prefetch call. You wouldn't want such prefetches to trigger new fetches neither.

Same thing for just any random rerender for any other reason since this will also be in the update path.

This comment has been minimized.

@BurntCaramel

BurntCaramel Dec 9, 2017

Could there be a method, say async load(props, state) (or fetch) that is used for both prefetching and actually updating the state like with componentDidMount() in the example? What would happen is that it returns a Promise resolving to key/value changes to be made to the state. (To make it more convenient, it could actually be an array of state changes, to make it easily used with Promise.all())

This would also solve the issue of the unmounting component attempting to setState(). In the case of an unmounted component, once the Promise from load() resolves, its result is just ignored.

This comment has been minimized.

@bvaughn

bvaughn Dec 10, 2017

Collaborator

Sounds similar to what @philraj proposed here?

I mentioned that I think it's also somewhat common for async requests to do things like dispatch Flux store actions on completion (rather than updating state). I think it's also common to make multiple async request, and for a single request to update multiple state keys. (I'm not commenting on the merits of these design patterns, just saying that I've observed them.)

I find myself hesitant about the idea of baking handling of this async promise-resolution into React core for some reason. I can't put my finger on why exactly and so I don't trust my opinion yet. (Maybe I worry about potential complexity or confusion. Maybe the idea is just too new to me.)

Edit Whoops. Looks like you already saw and responded to that thread. (I'm reading through notifications in chronological order, so I hadn't noticed.)

componentDidUpdate(prevProps, prevState) {
// Callbacks (side effects) are only safe after commit.
if (this.state.someStatefulValue !== prevState.someStatefulValue) {
this.state.onChange(this.state.someStatefulValue);

This comment has been minimized.

@sompylasar

sompylasar Dec 9, 2017

this.props.onChange

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

Whoops. Thanks.

@trueadm

This comment has been minimized.

trueadm commented Dec 9, 2017

Note: this isn't really that relevant to this PR proposal.

@markerikson In my opinion, something in state doesn't have to mean it anything to do with rendering. I think we have an opportunity to change that in the future and simplify things too. I attempted to tackle these problems with Inferno and they were very received – so I feel we can do better here.

@markerikson

This comment has been minimized.

markerikson commented Dec 9, 2017

@trueadm : I think the reason for that rule of thumb comes from this:

  • You shouldn't mutate this.state
  • Calling setState() queues a state update, but also a re-render
  • Updating a jQuery plugin or some other non-HTML-ish API isn't something that goes in render, so you need to apply the updates to that in another lifecycle method
  • Actual re-rendering is almost definitely wasted CPU cycles for this scenario
  • Thus, you don't want to call setState() at all, so that you don't trigger pointless re-renders

If you've got some ideas that work well as an alternative to that mindset, I'm certainly curious to see what you have in mind.

The purpose of this method is to initiate asynchronous request(s) as early as possible in a component's rendering lifecycle. Such requests will not block `render`. They can be used to pre-prime a cache that is later used in `componentDidMount`/`componentDidUpdate` to trigger a state update.
Avoid introducing any side-effects, mutations, or subscriptions in this method. For those use cases, use `componentDidMount`/`componentDidUpdate` instead.

This comment has been minimized.

@aickin

aickin Dec 9, 2017

I would consider pre-priming a cache (as was suggested in the sentence above) to be a side-effect, so I'm not sure this sentence means exactly what it currently says. Maybe it should say something more like "any non-idempotent side effects"?

This comment has been minimized.

@bvaughn

bvaughn Dec 9, 2017

Collaborator

Great suggestion! Thanks.

@BTMPL

This comment has been minimized.

BTMPL commented Jan 17, 2018

It will not be called in the current implementation but it is under consideration.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 17, 2018

The new lifecycle allows to use with functional components, as its static and js allows it. It must be documented what React will do in this case.

Lifecycle hooks apply to class components only. I think the existing documentation already makes this pretty clear. Once we add docs for the new (static) lifecycle methods we'll hopefully make that clear as well.

The docs are a community effort too, so if you think it's not clear enough- PRs are welcome.

Edit: I've added a TODO to my PR to detect and warn about this case in DEV mode.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 17, 2018

Will getDerivedStateFromNextProps be called for the initial render? The RFC only mentions it as a replacement for cWRP.

The RFC is slightly out of date with respect to a couple of things. (I'll probably update it once I've posted an implementation PR.) This is one of them.

Our current thinking is that getDerivedStateFromProps will be called after construction for the initial state.

@myitcv

This comment has been minimized.

myitcv commented Jan 17, 2018

Really glad to see this being discussed because it's a point we've debated/worked on a great deal 👍

This is a fairly lengthy thread so apologies if my comments end up repeating any of the points I've missed here or in the various code reviews.

PureComponent's

The one point I haven't seen discussed here is that of PureComponent's. We use PureComponent's across the board (because we use ImmutableJS exclusively) and one of the issues we've found with the current API is the sequence of events when it comes to updating props. The sequence of events from the perspective of a child component is as follows:

componentWillReceiveProps
shouldComponentUpdate
componentWillUpdate
render
componentDidUpdate

As things currently stand in React, even if props do not change (with respect to the definition of PureComponent) then the child's cWRP still fires. We handle this by actually proxying calls (we do a load of plumbing/overriding in a common base class) and only forwarding the cWRP call to our components if we determine the props have changed (calculated by the same shallow comparison as PureComponent's shouldComponentUpdate, because only in this situation would the subsequent shouldComponentUpdate return true). Whilst this does introduce some inefficiency, it does mean that cWRP has much clearer semantics in our components.

Common props handling

As things currently stand, we almost always delegate from a component's cWM and cWRP to a commonProps() method which has access to two helper properties: props and prevProps. props gives us the new/next props, prevProps (as its name suggests) gives us the previous props. prevProps can only be called from within (transitively) cWM or cWRP (although I think this restriction is superfluous).

(We implemented these as properties given the pre-existing props property incidentally.)

commonProps() is the point where we derive state from props (per this proposal), hence having access to the previous props is critical at this point (even though we know that the props have changed, because of the use of PureComponent).

Making setState sync

We have also made setState() a synchronous operation. I asked a question about why it was decided it should be async in the React Discussion Forums; there is also an open issue discussing this point.

We found that setState() not being synchronous leads to either mistakes or rather convoluted chained code where a value for nextState is passed from method to method, with one of those methods finally calling setState().

Memoized values

I've seen the discussion about putting memoizing logic into render(). This is fine except in those situations where you need the memoized values in places other than render() (i.e. callbacks etc). In which case these memoized values have to go into state. But this is fine: because your trigger for recalculating the memoized values is either a change in props (cWM/cWRP) or a change in state. In either case the component is going to re-render (by definition), so updating state further is effectively at no extra cost. Again this is where having setState() be synchronous helps to keep code simpler in our experience.

How this maps to the current proposal

All of that said, I have the following questions with respect to this proposal/thread:

  • getDerivedStateFromNextProps sounds very similar to the approach we follow with commonProps. Just to confirm, this method will only fire when the props have actually changed (see point about PureComponent's)?
  • I understand the point about componentDidMount being a better place to setup event listeners. As mentioned above, cWM is one of the entry points to commonProps (our method for deriving state from props). So are you proposing that the first time getDerivedStateFromNextProps would be called is after the initial render?
  • Any thoughts on the question about setState? Or indeed our approach of having made it synchronous? We can continue that discussion in the linked issue if you feel it's a better place for it, but as you can see we found it very much linked to the lifecycle points in this thread

Thanks in advance.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 17, 2018

getDerivedStateFromNextProps sounds very similar to the approach we follow with commonProps. Just to confirm, this method will only fire when the props have actually changed (see point about PureComponent's)?

This method will be called at the same time and in the same cases as componentWillReceiveProps is currently. This RFC does not propose any changes to the method with regard to shallow props comparison.

I understand the point about componentDidMount being a better place to setup event listeners. As mentioned above, cWM is one of the entry points to commonProps (our method for deriving state from props). So are you proposing that the first time getDerivedStateFromNextProps would be called is after the initial render?

As mentioned in the comment above yours, our current thinking is that getDerivedStateFromProps will be called after construction (for the initial state) as well.

Any thoughts on the question about setState? Or indeed our approach of having made it synchronous? We can continue that discussion in the linked issue if you feel it's a better place for it, but as you can see we found it very much linked to the lifecycle points in this thread

I understand why you mention it, but I don't think this (already pretty noisy) discussion is the best place to have this chat. 😄

@myitcv

This comment has been minimized.

myitcv commented Jan 17, 2018

Thanks for the quick response @bvaughn

This method [getDerivedStateFromProps] will be called at the same time and in the same cases as componentWillReceiveProps is currently. This RFC does not propose any changes to the method with regard to shallow props comparison.

Is there scope for extending this proposal to include consideration of PureComponent? Because otherwise logic within getDerivedStateFromProps will be called needlessly, negating the point/benefit of PureComponent. We can of course continue to proxy as I described, but I wonder whether we kill two birds with one stone and fix PureComponent as part of this proposal.

As mentioned in the comment above yours, our current thinking is that getDerivedStateFromProps will be called after construction (for the initial state) as well.

Sorry, I've just now seen that response. Makes total sense. Sounds like getDerivedStateFromProps will be a slot in replacement for our commonProps, notwithstanding the point about PureComponent above.

I understand why you mention it, but I don't think this (already pretty noisy) discussion is the best place to have this chat.

Noted 👍 - perhaps I can encourage you / @gaearon / others to chime in on either the linked issue or the React Discuss thread?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 17, 2018

Is there scope for extending this proposal to include consideration of PureComponent? Because otherwise logic within getDerivedStateFromProps will be called needlessly, negating the point/benefit of PureComponent.

It seems like you're concerned about the price of computing updated state from props if props haven't changed? A common pattern for dealing with this is to only update values in state if the corresponding props values have changed. (Assuming your component has more than one prop, you'd likely need to do this check anyway to ensure that the props your state care about have changed.)

I don't think this negates the value of PureComponent at all. Typically, the most expensive part of a component updating is the render (and all of the subsequent children updates).

As for the other discussion thread, I'm sure one of us will try to comment when we can. There's a lot of discussion threads and not many of us though. 😅 Thanks for bring it to attention again. Sorry!

if (this.state.externalData === null) {
// Prime an external cache as early as possible.
// (Async request would not complete before render anyway.)
asyncLoadData(this.props.someId);

This comment has been minimized.

@namuol

namuol Jan 17, 2018

I realize why this is being done, but should side effects in render really be encouraged for such a common pattern?

This comment has been minimized.

@ericvicenti

ericvicenti Feb 3, 2018

Member

Hopefully it will be pretty rare, and componentDidMount will work for most use cases.

But I wonder, why don't you begin loading async data in the constructor? Would that be encouraged?

This comment has been minimized.

@bvaughn

bvaughn Feb 3, 2018

Collaborator

I was hesitant to show this pattern on the RFC, because it's only intended for rare, advanced use cases (eg a library like Relay). I'm a little bummed that it's drawn a bit of negative attention, but I understand why. I should have probably labeled it much more clearly with a disclaimer or something.

That being said, you could trigger the load in the constructor. Side effects in the constructor feel worse to me than side effects in render, but neither is great. Timing wise though, it would be about the same. (render will be called synchronously, shortly after the constructor.)

@Ephem

This comment has been minimized.

Ephem commented Jan 17, 2018

I really approve of this proposal!

When it comes to componentWillMount and SSR specifically I have personally never seen a valid use-case for cWM in a SSR-app, but I have seen tons of invalid uses of it due to misconceptions. Marking it unsafe would definitely help clear this up.

@myitcv

This comment has been minimized.

myitcv commented Jan 18, 2018

@bvaughn

It seems like you're concerned about the price of computing updated state from props if props haven't changed? A common pattern for dealing with this is to only update values in state if the corresponding props values have changed. (Assuming your component has more than one prop, you'd likely need to do this check anyway to ensure that the props your state care about have changed.)

I don't think this negates the value of PureComponent at all. Typically, the most expensive part of a component updating is the render (and all of the subsequent children updates).

You make a very fair point; we do indeed end up comparing fields between props and prevProps to conditionally recalculate derived state.

So I think my "unease" simplifies then to the incongruence of getDerivedStateFromProps being called when, as a PureComponent, you know the props haven't changed (because shallowEqual(nextProps, prevProps) === true). Otherwise don't we end up having to document that it's called regardless?

But then perhaps I'm only pushing this because I was surprised when I first stumbled across this in the current implementation!

As for the other discussion thread, I'm sure one of us will try to comment when we can. There's a lot of discussion threads and not many of us though. 😅 Thanks for bring it to attention again. Sorry!

No apology needed :)

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 18, 2018

Otherwise don't we end up having to document that it's called regardless?

But then perhaps I'm only pushing this because I was surprised when I first stumbled across this in the current implementation!

For what it's worth, the docs do point this out 😄

componentWillReceiveProps()

Note that React may call this method even if the props have not changed, so make sure to compare the current and next values if you only want to handle changes. This may occur when the parent component causes your component to re-render.

@myitcv

This comment has been minimized.

myitcv commented Jan 18, 2018

For what it's worth, the docs do point this out 😄

🤦‍♂️ - I think I'll go and take a break :)

Thanks, I must have missed this before.

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 18, 2018

No worries. It happens~

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 19, 2018

This RFC has been implemented via facebook/react/pull/12028 which was merged this morning.

@acdlite acdlite merged commit f01ab98 into reactjs:master Jan 19, 2018

@bvaughn bvaughn deleted the bvaughn:static-lifecycle-methods branch Jan 19, 2018

@TrySound

This comment has been minimized.

Contributor

TrySound commented Jan 19, 2018

When we can expect 16.3 release?

@bvaughn

This comment has been minimized.

Collaborator

bvaughn commented Jan 19, 2018

To be determined.

React does not call this method before the intial render/mount and so it is not called during server rendering.
Note that React may call this method even if the props have not changed. If calculating derived data is expensive, compare next and previous props to conditionally handle changes.

This comment has been minimized.

@amannn

amannn Jan 22, 2018

@bvaughn The PR for React included
a note that getDerivedStateFromProps doesn't receive previous props. Should this sentence be updated?

This comment has been minimized.

@bvaughn

bvaughn Jan 22, 2018

Collaborator

The wording here is a bit ambiguous but I think that's okay. Current and previous props can be compared, as shown in this example

This comment has been minimized.

@amannn

amannn Jan 22, 2018

Ah, previous props in this case means props that have been synced to the local state and can be access via prevState, right? Alright then, I thought this was left over since I think the prevProps parameter got removed in a later iteration.

This comment has been minimized.

@myitcv

myitcv Jan 22, 2018

So @bvaughn does this mean that, given our previous conversation, I have to "sync" all my props to state in order to compare previous props with the new props? Feels like I'm missing something here.

This comment has been minimized.

@bvaughn

bvaughn Jan 22, 2018

Collaborator

Not all props, only ones that are directly used to derive state, and only if it's an expensive computation.

This comment has been minimized.

@gaearon

gaearon Jan 26, 2018

Member

Does storing "mirrored" props in state in addition to their derivatives not go against the React principle of state as a minimal representation?

It's a bit different though. You're not storing a "mirrored" version, you're storing the previous version. Which is a special case of an accumulation (that happens to completely discard the current value). Accumulation is exactly the purpose of getDerivedStateFromProps and since you already added it (presumably for accumulating something), adding another field to it doesn't hurt.

I guess one could say that a "mirrored" version would also be a special case of accumulation (that discards the previous value) but that's sophism IMO :-)

This comment has been minimized.

@billyjanitsch

billyjanitsch Jan 26, 2018

since you already added it (presumably for accumulating something), adding another field to it doesn't hurt

Yeah, I see what you mean. I just feel that it muddies the message of "don't put unnecessary stuff in state" where "necessary" no longer precisely means "used in render". Personally I find this a bigger / less intuitive exception to a rule than prevProps's nullability (the reason for which is pretty obvious -- no prevProps on first render). I suspect newer devs would struggle more with the former.

Anyway I see the trade-off, and I appreciate your willingness to explain! 😊

This comment has been minimized.

@gaearon

gaearon Jan 28, 2018

Member

I just feel that it muddies the message of "don't put unnecessary stuff in state" where "necessary" no longer precisely means "used in render".

We might actually revisit this one ^^

With async React it might not be safe to put certain things on the instance fields. See facebook/react#10580.

This comment has been minimized.

@myitcv

myitcv Jan 29, 2018

@bvaughn @gaearon I've tried to flesh out what I mean using a Typescript example - see this gist - to compare the two approaches.

I've extracted this example from a real life component that has more props, but hopefully you get the idea: any component would have to follow a similar pattern.

There are three files in the gist:

  1. the component written as per this proposal
  2. the component written assuming a signature of static getDerivedStateFromProps(nextProps: Props, prevProps: Props, initial: boolean): State
  3. the diff between the two

Notice file 2 is using a slightly different signature to the proposed in my last response; I agree with @gaearon the nullability issue is best avoided, hence prevProps is not nullable and an explicit boolean is provided to indicate whether it's the initial derivation or not.

Does the diff help to show what I mean about the boilerplate required when we have to "sync" to state?

Thanks

This comment has been minimized.

@bvaughn

bvaughn Jan 29, 2018

Collaborator

Notice file 2 is using a slightly different signature to the proposed in my last response; I agree with @gaearon the nullability issue is best avoided, hence prevProps is not nullable and an explicit boolean is provided to indicate whether it's the initial derivation or not.

Previous props would still need to be null or undefined in this case, no? Or even more confusingly, they would equal current props.

Anyway~ thanks for the feedback, Paul. I understand and appreciate your concern about verbosity. 🙇

This proposal was discussed in length in December, accepted, and merged into the React repo a couple of weeks ago. I'm going to lock down the thread at this point because I think we've already committed to this API.

Hope you understand!

componentDidMount() {
// Wait for earlier pre-fetch to complete and update state.
// (This assumes some kind of cache to avoid duplicate requests.)
asyncLoadData(props.someId).then(externalData => {

This comment has been minimized.

@TrejGun

TrejGun Jan 28, 2018

this.props ?

This comment has been minimized.

@bvaughn

bvaughn Jan 28, 2018

Collaborator

This PR was merged 9 days ago 😄 but that typo has been fixed in master.

@reactjs reactjs locked as resolved and limited conversation to collaborators Jan 29, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.