Issues with componentWillUpdate deprecations (state machines implementation)

[rosenfeld]

Hi, I'd like to discuss an use case for which I think the new lifecycle methods won't help demonstrating how it seems to me like a bad move to deprecate componentWillUpdate (or prefix it with UNSAFE_).

I have some sort of client-side router that uses the history API to change the query search to implement some sort of state-machine for a given component. Think of some UsersManagement component that could render actions such as list, edit and display actions. It has an "action" state and render() will dispatch to the proper method depending on what is the current action.

When there's an action change, some initialization is required and I'd like to reset some other relevant states. Currently, the only way I can think of implementing that is to store the previous "action" state in some instance property and compare the current action with the previous one in render(). When they are different I'd perform the changes to the other state attributes in-place since it's already too late to call setState() from render().

Even componentWillUpdate() is not ideal for that since I can't call setState from it either, but at least I wouldn't have to store the previous "action" state myself. With componentWillUpdate() I'd be able to compare the previous action state with the next one and modify the relevant state changes in-place before render is called, which seems like a bit cleaner approach.

I feel that something is missing in React and that it should be possible to react to state changes and perform further state changes before rendering without much hack. State machines should be possible even without something like Redux. Am I missing something obvious while trying to implement a state machine?

[gaearon]

Can you provide the minimal code demonstrating what you’re doing please?

[bvaughn]

Just in case it's unclear, mutating state- either this.state or prevState/nextState- was never supported. state should be treated as if it were immutable. (This is even more important with async rendering, because of how React rebases lower-pri state updates.)

[gaearon]

To me it sounds like you want to have a single entry point for state changes. There, use the setState functional form to get access to previous state.

performAction(action) {
  this.setState(prevState => {
    let nextState = { action };
    if (prevState.action !== action) {
      nextState.someField = null;
    }
    return nextState;
  });
}

It’s hard to say more without seeing your code. But if you need to adjust state based on state then you should do it when you set that state, not later.

[rosenfeld]

@gaearon I'm not currently at home and will only be back late in the night most probably, but I'll try to show some code example tomorrow.

Your example above makes sense. I guess I should be able to replace this.setState({ action: 'list' }) with something like performAction('list'). Once I'm back to home I'll try that approach in the actual code and if it's not that simple I'll let you know. I'll close that issue for now and if I think it's not that simple I'll add more details to the issue and reopen it.

@bvaughn I'm aware of that, but most of the render call rely on some state variables in order to render. Modifying the states in-place during render is way more simple than extracting some variables to something like "const someValue = isNewAction ? 'something' : this.state.somethingElse". It's much simpler to write something like:

if (hasActionChangedToList) {
  this.state.clients = [];
  this.state.somethingElse = ...
}

[bvaughn]

Right, right. I see how it could be more convenient to write that way, but I wanted to point out that there were subtle problems with mutation like that in the past (even without regard to async) which is one of the reasons why the will lifecycles have been deprecated and tagged as "unsafe". 😄

[gaearon]

You can also use something like https://github.com/mweststrate/immer if you want to write code in mutable style inside a pure updater function.

[gaearon]

That said you can already write code in a “mutating style” for top-level state keys in the updater function, like I show in #781 (comment). You’d need to be careful to not mutate the previous state with deeper updates though.

[rosenfeld]

Thanks for pointing out to immer. Seems like an interesting library even though I don't intend to use it right now. I actually liked the suggested approach in comment 781 and will try that once I'm back to home. Thanks. With that approach I wouldn't have to modify the previous state if I only care about mutating top-level state keys. For modifying inner states then I can always consider immer if I feel that modifying in-place could lead to unexpected behavior in my code. If I can easily save those 2k then I prefer to modify in-place, but if I suspect that could introduce subtle bugs, then I'd replace those in-line state changes with something like immer. Thanks for all your help and considerations :)

[thysultan]

@rosenfeld Was there any reason you used componentWillUpdate instead of componentWillReceiveProps since unlike the former, setState in the later updates/mergers the state update in-place* within the same render cycle(before render is called).

[gaearon]

cWRP wouldn’t fire on state-only changes.

[thysultan]

Was there any reason why setState in componentWillUpdate did not follow the same semantics as setState in componentWillReceiveProps.

[gaearon]

Probably that it was unnecessary to support setState in it? If you need to change state in response to changing state, it means that either one of those states is unnecessary (and can be eliminated), or that those states should have been computed together.

[rosenfeld]

@rosenfeld Was there any reason you used componentWillUpdate instead of componentWillReceiveProps since unlike the former, setState in the later updates/mergers the state update in-place* within the same render cycle(before render is called).

Just to be clear, I never actually implemented it using componentWillUpdate. I've thought about using that but since I never remember the actual names of the hook methods I always look at the documentation and this time I noticed the changes in the API, so I used the method I described in this issue, by storing the previous action and comparing against the current one in render(). But I thought that maybe it would be clearer if I used componentWillUpdate instead, without realizing setState is not supposed to be used in that hook either. As you can see, I'm still kinda new to React application development and I'm really giving it a try in the development of a new app I started to build since a few months ago.

But after thinking better about this I think Dan is right in pointing that the state reset should really happen once the action state is changed, which seems a bit obvious after this issue's discussion. It wasn't obvious to me while developing the application because I don't actually run this.setState({action: 'list'}) directly. I'd run something like this.navigateTo({action: 'list'}) instead, but it calls setState behind the scenes, so I should be able to abstract performAction anyway and use navigateTo in the implementation.

That should be enough, but if there's a catch I can't easily fix without those lifecycle hooks I'll get back to you.

[rosenfeld]

Hi @gaearon, I was finally able to take a closer look in the actual implementation and unfortunately the flow is not as simple as we have previously discussed. There's a top-level Router component which is responsible for rendering the current app, which is stored in Router's top-level "currentApp" state.

Suppose the current app is ClientsManager and that the current action is "list". When the user calls this.navigateTo({ action: 'edit', id: 3}), for example, the application, which has a reference to the router component instance, would call something like router.navigate('', { action: 'edit', id: 3});. The router would then generate the corresponding URL for that request, use the history.pushState to update the URL and finally set the currentApp and path states, which would then trigger a re-render for the router. The path is then provided to the current app through the "path" prop, which would then trigger a re-render in the current app.

When the current app (ClientsManager) gets the path property updated it would then perform a state change when it detects the action has changed, after extracting the action from location.search. At this point the state should be reset accordingly to the new action (or, maybe, in some rarer cases, depending on the previous action as well I guess).

With componentWillReceiveProps this flow is currently possible, however, I'm not sure how to implement this work-flow with getDerivedStateFromProps. Currently the former would set something like this.paramsState = this.getParamsState(nextProps.path) before calling setState in order to cache the results from parsing the URL to extract the request params. If I understood it correctly, I'd have to change this implementation to store the cached params in the state instead since I would no longer have access to "this". This is not ideal because shouldComponentUpdate would now have to perform a nested comparison of this new state which is not supposed to change.

Maybe you have some suggestion on what could be done to improve the situation in this scenario I described.

The idea behind the router is that once the user clicks on the edit button for a given client it would change the URL so that if the user then refreshes the browser they would still be editing that client rather than rendering the user's list action. However, when the application is already loaded we'd like those actions to feel instanteous. For example, once the user goes to the edit action and then hit the browser's Back button the router would take care of invoking the previous URL action ('list') but since the clients state was already set previously there's no need to fetch the list of clients again. So, it feels like a traditional state-less app with regards to navigation but can take advantage of a traditiononal SPA's speed improvements.

[bvaughn]

If I understood it correctly, I'd have to change this implementation to store the cached params in the state instead since I would no longer have access to "this".

Sounds right.

This is not ideal because shouldComponentUpdate would now have to perform a nested comparison of this new state which is not supposed to change.

If props changed in such a way as to require re-creating the paramsState object, wouldn't those changed props be sufficient to also cause shouldComponentUpdate to return true?

If it's possible that the props could change but the paramsState might not change, then you could perhaps memoize that bit in some way so that the wrapper paramsState object would only be recreated if its values changed.)

[rosenfeld]

If props changed in such a way as to require re-creating the paramsState object, wouldn't those changed props be sufficient to also cause shouldComponentUpdate to return true?

Currently, I'm not using something like React.PureComponent, but I understand I might if performance becomes an issue at some point. The PureComponent's shouldComponentUpdate implementation is a shallow prop AND state comparison, accordingly to the documentation, which makes sense to me. However, if I add a new paramsState state just for the purpose of caching the URL parsing results and only change some state with setState(), even when the props haven't changed the pure component would shallow compare the paramsState state as well, which is not ideal since it wouldn't change until the router loads another URL.

I realize I'm currently only suspecting about a theorical issue, since I neither use PureComponent at this point as I don't have any performance issues either right now and it's very likely the time spent to shallow compare that new state is neglitible, but I thought it might be interesting to share this use case anyway in case it might help understanding how componentWillReceiveProps is currently used.

[bvaughn]

However, if I add a new paramsState state just for the purpose of caching the URL parsing results and only change some state with setState(), even when the props haven't changed the pure component would shallow compare the paramsState state as well, which is not ideal since it wouldn't change until the router loads another URL.

Are you saying that paramsState could change in response to something other than a props change, in which case it should not require an extra re-render? (Sorry if I'm misunderstanding.)

I neither use PureComponent at this point as I don't have any performance issues either right now and it's very likely the time spent to shallow compare that new state is neglitible

Gotcha. Rendering is typically pretty fast. The React docs suggest to use shouldComponentUpdate only if you determine a specific component is slow (after profiling).

[rosenfeld]

Are you saying that paramsState could change in response to something other than a props change, in which case it should not require an extra re-render? (Sorry if I'm misunderstanding.)

No, I was saying that any call to setState would trigger a shouldComponentUpdate which, in the case of a PureComponent component would add a few more comparisons to check for changes in paramsState while it wouldn't actually change, leading to unnecessary comparisons, but again, it's very likely that this overhead would be neglitible anyway. It's more of a matter that it doesn't feel right than an actual performance issue ;)

The React docs suggest to use shouldComponentUpdate only if you determine a specific component is slow (after profiling).

Yes, that makes perfect sense and I don't actually think performance would ever be an issue with my current app with regards to client-side run-time performance. As I said it's more of some "it doesn't feel right" issue rather than an actual performance one :)

[bvaughn]

Gotcha. 😄

So with regard to this issue, are there any remaining concerns or questions (beyond the minor sCU perf thing we're talking about)?

[rosenfeld]

After thinking more carefully about this, there are other changes that would be required in order to convert my implementation to use a static method instead. Remember I call this.paramsState = this.getParamsState(nextProps.path). That means I'd have to change getParamsState to be a static method as well, which, for my use case, is fine as it doesn't really rely on anything specific to the component instance.

However, it's hard to predict whether this will always be the case. I have that feeling that one might actually need the this reference when implementing some state machine, when the next state will be a function not only of the next props and states but also of the previous ones as well, or maybe some instance's property as well. Yes, I don't currently have a concrete use-case, but I have that feeling that I might miss a reference to this in some cases...

[bvaughn]

Gotcha.

Removing access to the class instance (this) was an intentional design feature of the new lifecycle to prevent bugs caused by mutations.

If you have concerns about that aspect of the API- (specific ones are going to be better than fuzzy ones)- the RFC repo is probably the right place to discuss them. 😄

[rosenfeld]

Right, I'll try to make the changes to accomodate to the new API. If I notice something preventing me to achieve my goals with the new API (a concrete use case) I'll discuss that case in that repo. Thanks.

[bvaughn]

Thanks for the discussion 😄