Skip to content
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

Proposal: componentWillPrepareUnmount (continueCallback) #6067

Closed
kasper573 opened this issue Feb 18, 2016 · 10 comments
Closed

Proposal: componentWillPrepareUnmount (continueCallback) #6067

kasper573 opened this issue Feb 18, 2016 · 10 comments

Comments

@kasper573
Copy link

EDIT: After discussion in this thread I've amended componentWillRemount to the proposal.

I've been working a lot with animations in combination with react lately and while most lifecycle methods make transitions on components a walk in the park, I've struggled to get the scenario of unmounting a pleasant experience. In the end though I've come up with a lifecycle method proposal that could be incredibly powerful: componentWillPrepareUnmount (continueCallback) and componentWillRemount.

What they would do, when declared on a component, is to delay the actual unmount just as if nothing happened, until the component itself calls the provided callback, which then would resume the actual unmounting. If a component in a pending unmount state is restored before the actual unmount takes place, the unmount is canceled and the component treats the change as a regular update (componentWillUpdate fires, etc), with the addition of componentWillRemount being called to give you the chance to react specifically to this event.

Edge case mitigation: Ancestors override descendants. If an ancestor of a component using "unmount preparation" unmounts the descendant unmounts instantly without preparation, unless the ancestor also utilizes componentWillPrepareUnmount, which would give it some arbitrary time to prepare.

This would allow us to gracefully perform an animation on our components right before they're really unmounted, letting component specific code decide how it should disappear.

For example:

var styles = {
    // .. some styling
};

var tweenOptions = {
    // .. some tween options
};

/**
 * A menu item that fades in and scales up on mount 
 * and then fades out and scales down on unmount.
 */
class MenuItem extends React.Component {
    componentWillMount () {
        this.tweens = {
            opacity: new Tween(0, tweenOptions),
            scaleY: new Tween(0, tweenOptions)
        };
        this.animateShow();
    }

    componentWillPrepareUnmount (continueCallback) {
        Promise.all([
            this.tweens.opacity.to(0),
            this.tweens.scaleY.to(0)
        ]).then(continueCallback);
    }

    componentWillRemount () {
        this.animateShow();
    }

    animateShow () {
        this.tweens.opacity.to(1);
        this.tweens.scaleY.to(1);
    }

    render () {
        return (
            <surface style={{...styles, this.tweens}}>
                {this.props.label}
            </surface>
        );
    }
}

And the usage:

class Menu extends React.Component {
    render () {
        var itemElements = this.props.items.map((item) => {
            return <MenuItem key={item.id} label={item.label}/>;
        });
        return (
            <surface style={{flexDirection: "row"}}>
                {itemElements}
            </surface>
        );
    }
}

Menu.propTypes = {
    items: React.PropTypes.arrayOf(
        React.PropTypes.shape({
            id: React.PropTypes.any,
            label: React.PropTypes.string
        })
    ).isRequired
};

What do you think?

@jimfb
Copy link
Contributor

jimfb commented Feb 18, 2016

At first glance, that actually seems pretty clever! cc @sebmarkbage @vjeux

@sebmarkbage
Copy link
Collaborator

This is essentially what transition group is about. Except that one doesn't allow an abstraction in the child hierarchy to be in control over the animation. Consequently you want something like this.

However, if you do this, then the parent of this and the parent of that will have to be aware at some stage that this thing is still alive so that it doesn't delete it from its set. That's what "completion propagation" as mentioned here: #2928 is all about. To make this work we would also need some kind of completion propagation so that a parent can know when a child is supposed to remain undeleted and when it is safe to remove it from the parent.

Also, what happens if the same key gets restored while this is still happening? Are components above it? E.g. is a parent of this allowed to update its state while this is happening?

While something like this API is certainly something we need, it has a few dependent complexities to make that work.

@kasper573
Copy link
Author

@sebmarkbage Good point about restoration. I'm thinking that it should be safe to assume a re-attached component can cancel the ongoing unmount preparation (by ignoring the continueCallback when it's called) and do one of the following:

  1. Imitate the mount procedure, ultimately triggering componentWillMount, etc, once more. This seems a bit aggressive though and made me think of 2.
  2. The component instead gets updated like any other react update, though with a special follow up: another lifecycle method componentWillRemount, which would give you the granularity to customize this event (although most scenarios will probably run the same code you have in componentWillMount).

@satya164
Copy link

Probably the component also needs to know if the removal was interrupted, so that it can animate back to previous state.

@kasper573
Copy link
Author

@satya164 @sebmarkbage I amended the proposal with componentWillRemount addressing the issues you've mentioned.

@syranide
Copy link
Contributor

Isn't this essentially:

<MyList>
  <MyAnimatedChild
    key={...}
    onRemoveAnimationComplete={this.handleDeleteChild...}
    isRemoving={this.isRemoving[...]}
  />
  ...
</MyList>

From this you can deduce componentWillPrepareUnmount + componentWillRemount in the MyAnimatedChild component and when the child is finished animating it calls onRemoveAnimationComplete (i.e. continueCallback) which tells the parent to delete it from the list of children. Perhaps I'm missing something, but it sounds like this is what's being suggested (*) and this should be possible to design as a reusable baseclass/mixin/whatever today, this issue recommends it for core instead?

(*) Because the parents needs to keep track of children being removed and also keep rendering them somehow, this cannot be handled transparently in the generic case. One can also provide an abstraction for simplifying dealing with (or even hiding) handleDeleteChild and isRemoving.

@kasper573
Copy link
Author

@syranide It's true you can achieve the same result in other ways. Another great way is to use the ReactTransitionGroup addon, but with both that and your solution some wrapping logic is required. The idea of my proposition is to be able to perform an unmount transition solely using the component and no wrapping.

Also, can you explain why this cannot be handled transparently in the generic case?

@syranide
Copy link
Contributor

Also, can you explain why this cannot be handled transparently in the generic case?

A parent cannot simply stop rendering a child and still have the child remain rendered (and animating), at the very least the child has to be ordered among the other children and without the child being actively rendered that information is simply not available to React.

Consider three children, D,G,T, G is removed and starts animating away D,[G],T. A new child is added and rendered as D,L,T, where should the implicit [G] being removed be placed? Before or after L? That information is not available unless the parent is still rendering G until it's entirely removed from rendering.

So unless I'm missing something, your proposal requires the same onRemoveAnimationComplete + isRemoving that I have in mine, one way or another. It cannot be transparent to the parent. So it seems to me that having this in core doesn't actually provide any obvious benefit?

EDIT:

but with both that and your solution some wrapping logic is required.

My solution does not require wrapping logic, it requires having the child extend a different base class or apply a mixin unless you want to reimplement the shared logic every time.

@gaearon
Copy link
Collaborator

gaearon commented Feb 19, 2016

@chenglou thought about this a lot (which resulted in react-motion). I found his thoughts on animation quite insightful.

@jimfb
Copy link
Contributor

jimfb commented Feb 24, 2016

After some discussions with Sebastian, and thinking on this issue, I agree that this doesn't elegantly handle the edge cases for the reasons mentioned above (classic problem with proposed solutions to animation). For this reason, I'm going to close out the issue. But feel free to continue the discussion on this thread, and we can re-open if our thinking changes dramatically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants