Endless componentWillUnmount loop with <Transition> when another update is triggered

[Prinzhorn]

Here's the reproduction, it's a plain create-react-app https://github.com/Prinzhorn/react-spring-will-unmount-bug/blob/master/src/App.js

Quick start

git clone git@github.com:Prinzhorn/react-spring-will-unmount-bug.git
cd react-spring-will-unmount-bug
npm i
npm start

Open your JavaScript console, run the example app and hit the button "Toggle Panel" twice. When the panel is closing you can see an endless loop of:

componentWillUnmount
componentDidMount
render
componentWillUnmount
componentDidMount
render
componentWillUnmount
componentDidMount
render
componentWillUnmount
componentDidMount
render

(it's not actually endless, React will kill it after 1000 runs).

If you remove line 33 this.props.onDataChange('bar'); the endless loop is gone, however the lifecycle hooks are still not correct

render
componentWillUnmount <--- triggered when the animation starts
componentDidMount
componentWillUnmount <--- triggered when the animation ends with no render call in-between

Details

I have a side-panel in my app with some settings (text inputs and such). I want to save the changes once the panel is closed, that's why I pass the updated data to a central store from inside componentWillUnmount. However, the panel(s) are controlled by a react-spring <Transition> which does weird things. The updated data will cause the <Transition> (or the containing component) to re-render but always with an empty array (since the panel is currently closing). But it is still causing the to-be-removed panel to be mounted/unmounted endlessly.

[Prinzhorn]

Something is super fishy with the lifecycle hooks. Here's what happens when I keep smashing the Toggle Panel button with line 33 removed

http://www.prinzhorn.me/vokoscreen-2018-06-28_12-22-37.mp4

I don't think they should stack/queue up like that.

[Prinzhorn]

Looks like some discussion regarding the second issue is already happening in #134, so we should focus on the endless loop in this issue.

[drcmda]

It looks like it pushes the same key for all elements, can that be?

[Prinzhorn]

It looks like it pushes the same key for all elements, can that be?

What is pushing keys to where? If I log panel and keys here they're always an empty array during the endless loop

https://github.com/Prinzhorn/react-spring-will-unmount-bug/blob/0108dbe61927d164e001876e91ae342970d8c5d6/src/App.js#L90-L94

[drcmda]

I’ll take a look later on.

[will-stone]

I found going back to v4 helped. Whilst v4 doesn't seem to be able to do the no-key feature, this worked just fine:

<Transition
    keys={showLogo ? "logo" : id}
    from={{ opacity: 0 }}
    enter={{ opacity: 1 }}
    leave={{ opacity: 0 }}
  >
    {showLogo
      ? styles => <Logo key="logo" style={styles} />
      : styles => (
          <div key={id} style={styles}>
            ...
          </div>
        )}
</Transition>

[drcmda]

@Prinzhorn could you map this into a codesandbox? A lot of issues crept up while i was away, would make it easier on my part to get on top of it all. :-)

[drcmda]

@Prinzhorn i've looked through it, it's a bug in your code. First, you shouldn't use transition for a sidebar, it's a basic spring you're looking for. But since you do, you actually mount and unmount that bar on every change of this.state.showPanel. The infinite loop that's now caused is also in your app, you call this.props.onDataChange('bar') in componentWillUnmount, which then calls setState.

Here's a fixed example:

import React, { Component } from 'react'
import { Transition, animated } from 'react-spring'
import logo from './logo.svg'
import './App.css'

class Panel extends Component {
  constructor(props) {
    super(props)
    this.state = { value: props.data }
  }

  handleChange = e =>
    this.setState({ value: e.target.value }, () =>
      this.props.onDataChange(this.state.value)
    )

  componentDidMount() {
    console.log('componentDidMount')
  }

  componentDidUpdate() {
    console.log('componentDidUpdate')
  }

  componentWillUnmount() {
    console.log('componentWillUnmount')
  }

  render() {
    console.log('render')
    return (
      <animated.div style={this.props.style} className="panel">
        <input
          type="text"
          value={this.state.value}
          onChange={this.handleChange}
        />
      </animated.div>
    )
  }
}

export default class App extends Component {
  state = { showPanel: false, data: 'foo' }

  togglePanel = () =>
    this.setState(prevState => ({ showPanel: !prevState.showPanel }))

  handleDataChange = data => this.setState({ data })

  render() {
    return (
      <div className="App">
        <h1>{this.state.data}</h1>
        <p className="App-intro">
          <button onClick={this.togglePanel}>Toggle Panel</button>
        </p>
        <Transition
          native
          from={{ x: '-100%' }}
          enter={{ x: '0%' }}
          leave={{ x: '-100%' }}>
          {this.state.showPanel &&
            (({ x }) => (
              <Panel
                style={{ transform: x.interpolate(x => `translate3d(${x},0,0)`) }}
                data={this.state.data}
                onDataChange={this.handleDataChange}
              />
            ))}
        </Transition>
      </div>
    )
  }
}

But like i said, i wouldn't cause the Sidebar to re-instanciate, it isn't good for performance. Better use Spring and maybe switch it off (display: none) when it's out, but you should let critical components always linger around in react and the dom in general because the cost of causing a full nested tree re-do is expensive.

[drcmda]

@will-stone would you open a new issue with an isolated csb example? until now i can't see a problem in transition, it behaves as it should. i'd need an example to look into it.

[Prinzhorn]

@drcmda thank you very much for taking the time to take a closer look. I have to take a closer look at your proposed solution but two things from the top of my head:

1. I'm not actually using this for a single sidebar. In reality the sidebar can be navigated and have nested children (similar to a navigation context in an app). That means there is a stack of open panels and a back-button, which pops one off the stack. Hence the use of <Transition> and not <Spring> (the nested panels/cards are stacked on top of each other and popping them fast gives the nice Transition effect). The example is just the minimal code the reproduce the issue.
2. The proposed solution does not use componentWillUnmount but I deliberately put the onDataChange in there. It's the exact moment I want the data to be passed to the parent. As I said the side-panel contains text inputs and controlled inputs need a corresponding state. I hold this state as low as possible right on the panel. However, once the panel is closed I need to notify the parent of the new data, it does not need them any sooner. In your case you are passing the data up for every keystroke. How will I know when the nested panel is closed? Using componentWillUnmount sounds like the perfect place for what I want to achieve and it makes it completely transparent to the parent. If I turn the control upside down then the parent would need to hold the state for every single controlled input in every single nested panel and thus needs to know about the panels implementation details.

[drcmda]

@Prinzhorn I'd put the sidebar into a normal spring, a button controls its visible state, similar to this demo: https://codesandbox.io/embed/zl35mrkqmm

As for the rest, could you make a demo without any animation? children notifying their parents is usually not a good idea, unless you do it with context, but setStating into the parent in the middle of an unmount is probably going to be problematic. The infinite loop i debugged in your example stemmed from that.

If you can, codesandbox would make this quick and painless on my side.

[Prinzhorn]

I'd put the sidebar into a normal spring, a button controls its visible state

like I said there is not a single sidebar. I'll try to put a demo up.

As for the rest, could you make a demo without any animation?

That would be my exact demo but just remove the <Transition> and render the panel(s) directly. That's kind of the selling point of <Transition> for me that you can just wrap some existing children which would otherwise enter/leave without a transition. I'll try to make a demo which better showcases what I'm doing.

children notifying their parents is usually not a good idea

It was my understanding that this in the primary way to pass data with plain React (pass a function down using props which the children can use to pass data back up). The new Context API merely makes this more comfortable by skipping a few intermediate parents and making it less verbose to pass things up and down, it's still the same thing. I am in fact using context and don't notify the parent directly, the parent just happens to consume it. If you abstract this away using something like Redux it gets worse because you can pass data to a parent without knowing it (all you do is update the store and don't care who's watching it).

setStating into the parent in the middle of an unmount is probably going to be problematic

React itself guarantees that componentWillUnmount is only ever triggered once. From the name componentWillUnmount I also thought it would not be in the middle of an unmount but before.
I can try to move the notification code to some other place, e.g. the button click handler. But since the button click triggers the unmounting it will likely still happen synchronously within the same tick so I don't gain anything.

Thanks again for taking the time. Maybe I'm using the <Transition> API wrong but I'll try to put a demo up which resembles what I'm actually doing.

[Prinzhorn]

I've created a more realistic test case. It is a bit more complex but we have already established that the issue can be reproduced with the basic test case in my first post. This demo shows a use-case that cannot trivially be replaced with a <Spring> without re-implementing most of the <Transition> logic.

So let's say in my App I have an article object. An article has a text body, a title, a meta description, a background color, some fonts, a header image and a lot more. I store these in a single object which is synced with a database. This article being edited is available throughout the app via context, a redux state or whatever you prefer. The demo uses a plain state.article at the App.js level. I have a side panel where all these attributes of the article can be edited in a nicely organized fashion. The side panel itself has a navigation context and a back button to hierarchically organize all of the article details that can be edited. Every side-panel is independent of each other and knows about the current article object. Again, this could be a context, a redux store or for simplicity it's just an article props. Every panel (and other parts of the app) are free to mutate the article object and pass it to the context, store or whatever (in the demo it is passed using this.props.onArticleChange). It then periodically gets synced to the server.

The demo shows three things that can be edited which are organized hiercharchically to demonstrate why I use <Transition>. <Transition> gives me the nice transitions I want, even when smashing the back-button multiple times (e.g. when you navigate to root->meta->title and hit back two times). What you suggested was that I could use <Spring> and just hide every panel without actually destroying it. But that defeats the purpose. Some panels make network requests or open WebSockets. I need to create/destroy them the same way I would if I would not use transitions at all. For that reason I need a reliable componentWillUnmount behavior (regardless of the endless loop we are discussing here, the lifecycle methods are also called multiple times in an unexpected way).

Uncomment line 65 in App.js for the endless loop.

https://codesandbox.io/embed/r1o71p7x1m?module=%2Fsrc%2FApp.js

[drcmda]

@Prinzhorn I tracked it for a while, i am still stuck - relying on lifecylces/componentWillUnmount to communicate back to the parent which then calls setState is extremely unconventional. If you find another solution for that your problem is fixed.

There's something in Transition that instanciates a leaving parent with a new key. "metadata" gets renamed to "metadata_" for instance. This explains the mounts/unmounts. This has to be done, the old key can't be used lest the same object re-appears and stops the old one from transitioning out. This is probably what's causing the loop, but on react-springs side, what it does is fine. It's the componentWillUnmount/parent.setState thing that shouldn't be done.

[drcmda]

@Prinzhorn perhaps i should give transition a do-over, there must be another way. Currently leaving transitions are, as i explained, re-keyed in order to prevent incoming transitions with the same key to obstruct the leaving animation. But perhaps it would be possible to re-key transitions straight up when they're added by looking if their key is already present. Transition unfortunately is the most complex primitive with the most side effects - i run some tests and see what i can do.

[Prinzhorn]

@drcmda thanks again. I do have very specific thoughts about that but I don't have time today to look into it. I'm sure we can find a clean way to handle that.

[Prinzhorn]

This has to be done, the old key can't be used lest the same object re-appears and stops the old one from transitioning out.

I think it all boils down to this sentence. The fundamental question is if this assumption/behavior is correct. Is there really an "old" and a "new" one?
Let's say a child with key K is rendered while a previous child with key K is transitioning-out. Should we really render a new element or is this fundamentally just a case of passing the right props to the existing child and changing the animation (which I assume is handled by spring)? Since we're already relying on keys and keys are used to identify things it seems odd that we now have two objects which were originally given the same key by the user of <Transition>.

1. I render [A, B, C]
2. They all transition-in
3. Later I render [A, B]
4. C starts to transition-out (it's still mounted and componentWillUnmount hasn't fired)
5. During the transition I render [A, B, C] again
6. C, which is currently transitioning-out, stops half-way and does the transition-in animation. This is something that <Transition> doesn't have to care about in detail, it just changes the prop on the <Spring> that wraps C.
7. We end up with the same thing being rendered as after 2. has finished. From the perspective of C nothing really happened (not even componentDidUpdate if native is used) since it has never actually been unmounted.

All that <Transition> would really do is delegate to <Spring> and then actually unmount a component in onRest if the direction was out (we keep a list of keys that should be removed, we can compare what keys were added/removed in componentDidUpdate of <Transition>). I think if we key the <Spring> with the key of the child it wraps then React will handled most of the nitty-gritty for us.

All that being said, I haven't seen the react-spring code and maybe I'm missing something.

[will-stone]

@drcmda Apologies for taking so long to set this up but here's my sandbox of this issue: https://codesandbox.io/s/1vjk1jn4ll

In the Logo component, you can see that the animation stops at the end (using animation-fill-mode = forwards). However there is a flash of its initial state as it fades out because it has been remounted.

[will-stone]

And here's the working comparison when using v4 of React-Spring: https://codesandbox.io/s/pw318qn21x

[drcmda]

@will-stone yes, i see, that's unfortunate. The re-keying essentially re-mounts your Logo component which prompts the browser to run the css animation again. v4 didn't re-key, but then again, it couldn't guarantee an uninterrupted fade-out if an element with the same key entered again.

I would like to support both, guaranteed exit without remount. But this isn't trivial - i'll need some time to get this right.

[will-stone]

Thanks @drcmda I ended-up making the animation return to the start as a sort of hack. In the end, I preferred that animation, anyway, as it worked well with the website: https://will-stone.github.io/SpotSpot/

[drcmda]

Looks nice, but i can see now how Transition should support it - the css reset is a pretty serious strike against the current model.

[drcmda]

could you guys try 5.4.2-beta1? I implemented the key-deriving part from scratch and now it doesn't re-key exit transitions.

edit:
@will-stone i tried your demo with it and it seems fine now.

@Prinzhorn yours seems to work as expected, too :-)

[drcmda]

I guess i just release and close this

[will-stone]

@drcmda Looks good to me. Thanks!

[Prinzhorn]

@drcmda thanks, the endless loop is gone! Now the only thing that is left is the fact that componentWillUnmount fires twice because a second component is instantiated (that's what happens in my local project).

However, looking at my codesandbox it looks like now other things are broken: components are no longer unmounted after the transition has ended

Steps to reproduce

1. https://codesandbox.io/s/r1o71p7x1m
2. Click "Meta Data"
3. Click "Title"
4. Click "Back"
5. The "Title" Panel is still in the DOM even though the only key that was rendered was "metadata" (see console: Rendering nested panels: metadata and it was Rendering nested panels: metadata, title before)

I think before continuing we need tests for Transition (independent from #102, these wouldn't test the animations itself but how the lifecycle methods behave).

[drcmda]

@Prinzhorn this could be a react error, update to react 16.4. They had a bug before where getDerivedStateFromProps isn't consistently called. I've seen the effect of it before, elements stay put after the exit phase. After the update this shouldn't happen.

Tests would be great as Transition is the most complex primitive that can cause the most side effects.

Edit:

tried it, yep, they're gone now.

[Prinzhorn]

tried it, yep, they're gone now.

maybe I'm doing something wrong, I'm not used to CodeSandbox. I've changed the dependency to 16.4.1 and to make sure it worked I render React.version in the title of the app. However, the panel is still there? https://codesandbox.io/s/r1o71p7x1m

Edit: here's a video http://www.prinzhorn.me/vokoscreen-2018-07-23_17-38-28.mp4

[drcmda]

react-dom is still on 16.3.1 in that box, you need to update both react and react-dom, then it should work.

[Prinzhorn]

@drcmda nice, seems to work!