Add transition status param to transition end listener

[mvasin]

This is a WIP PR that addresses #389.

I started with tests (in TDD spirit). Any feedback early on is welcome!

[jquense]

Seems like the the right tests, I wonder tho if we should make a breaking api change here and have the order be node, status, done. Callbacks that aren't last is really unidiomatic, which is why it may be worth the break to avoid an ugly api

[mvasin]

@jquense I don't mind placing the callback last. But before I'll start to reorder params, please confirm that you still believe that breaking addEndListener API is better then adding done callback to onEntering and onExiting hooks (where the callback would naturally go last).

[jquense]

yes we do not want to add any callbacks to onEntering and onExiting, they belong properly in addEndListener

[mvasin]

@jquense I think I'm mostly done with the PR, please have a look.

Here's GSAP and the updated RTG showcase: https://github.com/mvasin/rtg-gsap-example
I couldn't make it work on codesandbox due to codesandbox/codesandbox-client/issues/1036, so try it locally.

[mvasin]

@jquense GSAP use case does not oblige to provide a DOM node; it can animate a bunch of elements by their classes or ids (and not necessary this node element among them), so I removed this check.

[jquense]

I don't know that makes sense in terms of the API. GSAP can animated stuff via other means but the idea here that animation is localized to the node associated with the component (or it's children). I find the example referencing IDs confusing personally. Why use this component at all if that's the approach?

[jquense]

I would add a but unrelated that this check was added to prevent other bugs , so I'd like to keep it

[mvasin]

The whole idea of this PR is letting work with multiple transitions on multiple nodes. And you wouldn't know which will transition last (you definitely don't want to hardcode it).

I use <Transition /> to transition between the routes (URLs). That's how it works for me. If you have a better idea on how to dismantle previous page and assemble next page both consisting of multiple moving elements having funky transition timing dependencies, please share.

Regarding the potential bugs, if tests pass, there are no bugs. If something will go wrong, let's discover it, write a failing test for it and fix it. Otherwise it's a road to poor and frozen code, when everybody is afraid to touch anything, like in this case.

[mvasin]

@jquense According to git blame, you are the one who wrote this line. If you have no idea what it stands for, let's remove it.

[mvasin]

I believe you and am ready to put it back. But it would be a healthy practice to write a test that fails without this conditional and passes with it. Can you write one?

And I'll try to write a test for GSAP use case that fails with this conditional. :) Hopefully we'll find a middle ground.

[mvasin]

I added tests that ensure addEndListener is triggered even when <Transition>'s child renders nothing to the DOM. I think this behaviour is the most versatile; it works well when a child only orchestrates animations and does not render any DOM elements by itself.

It does fail when the if (node) conditional is put back.

An interesting issue arose - have a look at the failing test on Travis. If you accept that <Transition> can have a child that renders nothing to the DOM, I'll investigate it.

[jquense]

looking good, tho I don't think i understand why the "renders no node" case is important here. It looks like the example you've provided does render nodes, e.g. the page associated with the route.

[mvasin]

I've used () => null component that does not render to DOM and imitates a GSAP component that does timeline animation by ids/classes of other components and does not render to DOM anything on its own.

When <Transition> contains () => null (named wrapperDOMlessChild in the tests), you can see in the failing test that when exiting starts, <Transition> is not in entered state as expected, but in exited instead.

In my own project currently I do render inside <Transition>, but I'd cover this edge case because I think it's a valid.

[jquense]

Can we split that that case into another PR, and keep this focused on adding the status to the endListener? I think we are getting a bit sidetracked on a thing that wasn't the original functional addition

[mvasin]

@jquense Alright, I've finished the PR with the idea that a child of <Transition> renders something to the DOM. I agree that we can finish off this DOMless child case in a separate PR if needed.

[mvasin]

I've discovered a pattern that lets figure out the current transition status using just the current RTG API:

<Route path="/some-path">
  {({ match }) => (
    <Transition
      appear
      mountOnEnter
      unmountOnExit
      in={!!match}
      addEndListener={(node, done) => {
        const status = !!match ? 'entering' : 'exiting';
        someTransitionFunction(node, status, done);
      }}
    >
      <ChildComponent />
    </Transition>
  )}
</Route>

It turns out, if you're able to check <Transition>'s in prop inside addEndListener, you're good to go!

Here's the full working example: https://codesandbox.io/s/3yj7qyw9qm

This rises a huge question whether this PR should actually be merged or it's better not to break the API. I lean to the latter unless we'll bump into a use case where figuring out the current transition status inside the function passed to addEndListener is a problem.