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

RFC / useTransition 🤯 and possible improvements #665

Closed
drcmda opened this issue May 6, 2019 · 21 comments
Closed

RFC / useTransition 🤯 and possible improvements #665

drcmda opened this issue May 6, 2019 · 21 comments
Labels
kind: request New feature or request that should be actioned
Milestone

Comments

@drcmda
Copy link
Member

drcmda commented May 6, 2019

As we all know, transitions are hard. 🤯

If you want put your ideas in here and we'll discuss them, no matter how wild they are.

What people don't easily understand is that transition is a value retainer, it's not magic, it just saves a value (or an array of values) and looks for the addition or removal of keys. On changes it can retain the old value and pass it to the view, so that it can fade out an old state. So that has to be expressed better in the docs for sure. But the api is a little weird, too.

  1. A transition result is always an array. In theory this probably cannot change since there are reasons for it, it has to be documented better. Maybe it can be abstracted, though.

  2. That array contains objects made of { item, key props }, which is weird. Often people see "item" down in the view section and don't know where it comes from. This can be changed.

Some ideas:

// Proposed shortcut signature, if items are atomic (as in, items serve as keys)
const items = ['each', 'item', 'can', 'serve', 'as', 'a', 'key']
useTransition(items, config)

// When items aren't atomic it needs a key-accessor to recognize items
const items = [{ id: 0, text: 'hello' }, { id: 1, text: 'world' }]
useTransition(items, item => item.id, config)

// Function abstraction?
// Shorter, saves user from worrying about keys, retained item can be named properly
const transition = useTransition(visible, { from: {}, enter: {}, leave: {} })
return transition((visible, style) => visible && <a.div style={style}>hello</a.div>)

The current solution:

const transitions = useTransition(visible, null, { from: {}, enter: {}, leave: {} })
return transitions.map(
  ({ item: visible, props, key }) =>
    visible && (
      <a.div key={key} style={props}>
        hello
      </a.div>
    )
)
@drcmda drcmda added the kind: request New feature or request that should be actioned label May 6, 2019
@bmcmahen
Copy link

bmcmahen commented May 6, 2019

One thing that would be amazing (and maybe you can already do this, but i just don't know how) is to be able to imperatively update and access the spring that results from the transition. I've been finding it difficult to combine transitions with gesture based controls, so I usually just end up using useSpring and manually unmounting using onRest callbacks.

Maybe something like this (pseudo code)?

return transition((visible, style, set) => {
 const someGestureFn = (x) => set({ x })
 return visible && <a.div style={style}>hello</a.div>
})

@drcmda
Copy link
Member Author

drcmda commented May 6, 2019

@bmcmahen indeed, useTransition is the only hook that doesn't support set. We could extend the signature as we have done with the other hooks:

const transitions = useTransition(items, config)
const [transitions, update] = useTransition(items, () => config)

Another idea would be to normalize the api and remove the two signatures in favour of an optional dependencies array, like useMemo/useEffect/useCallback:

const [props, set] = useSpring({ opacity: show ? 1 : 0 }, [show])
const [springs, set] = useSprings(items, item => ({ opacity: show ? 1 : 0 }), [show])
const [springs, set] = useTrail(items, { opacity: show ? 1 : 0 }, [show])
const [transition, set] = useTransition(items, { from: { x: 0 }, enter: { x: 1 } }, [])

I would like this very much. Would reduce the api surface and would help us to figure out changes.

@aleclarson
Copy link
Contributor

aleclarson commented May 6, 2019

// Proposed shortcut signature, if items are atomic (as in, items serve as keys)
const items = ['each', 'item', 'can', 'serve', 'as', 'a', 'key']
useTransition(items, config)

👍 on the new useTransition overload

The proposed "function abstraction" (the third example in the OP) is kinda like renderprops for hooks. I like how it abstracts away the responsibility of passing a key, but maybe it's too much magic for some people? I say go for it.

I think making the transition callback's argument an object is the right choice, because it encourages everyone to use the same variable names (when destructuring the property names). Also, it allows for more properties to be added in the future if necessary. We could even add a setter method to the object argument by default.

 

Another idea would be to normalize the api and remove the two signatures in favour of an optional dependencies array, like useMemo/useEffect/useCallback:

const [props, set] = useSpring({ opacity: show ? 1 : 0 }, [show])
const [springs, set] = useSprings(items, item => ({ opacity: show ? 1 : 0 }), [show])
const [springs, set] = useTrail(items, { opacity: show ? 1 : 0 }, [show])
const [transition, set] = useTransition(items, { from: { x: 0 }, enter: { x: 1 } }, [])

I would like this very much. Would reduce the api surface and would help us to figure out changes.

This is already added to useSpring and useSprings in v9 (thanks to @Nizarius).

Except currently, it doesn't cause a setter to be returned, which I'm a fan of. I prefer the function syntax for "setter mode".

@biowaffeln
Copy link

Hi Paul! Thanks for creating this awesome animation library. I've been trying to wrap my head around react-spring for a couple of days now, so here are a few observations from a newbies perspective, hope that helps.

First of all, I was somewhat confused about the different function signatures with set. Why would you need that in the first place? Can't you just always return an array [props, set] and let the user ignore set if they don't need it?

// either
const [props] = useSpring({opacity: show ? 1 : 0})
// or
const [props, set] = useSpring({opacity: 0})
set({opacity: 1})

This feels much more natural to me, kinda looks like the useState-Hook if you squint a bit.

Second, the useTransition-API was really confusing and unintuitive for me as a beginner as well. I had a simple use case (fading out a single component on unmount) that I wanted to implement, but which felt very awkward with the current API. I kept asking myself things like "why does it return an array? I just want to animate a single component..." or "what is this whole key thing about?". I think your function abstraction suggestion would definitely be a move in the right direction.

That being said, I really enjoyed the solution for animating transitions that react-pose went for, which is extremely simple and elegant. I wonder if something like that would be possible with react-spring, too. It might look something like this (just spit-balling, not sure if that could even work with the way react-spring is built):

import { animated, useTransition, Transition } from "react-spring";

const [props, set] = useTransition({
  from: {}, enter: {}, leave: {},
});

return (
  <Transition>
    {visible && <animated.div style={props}>hello</animated.div>}
  </Transition>
);

Transition would be responsible for handling its childrens mount/unmount-animations, similar to react-pose's PoseGroup.

Additionally, if you have a list of elements, you could have a useTransitions-Hook which would be a sort of transition-equivalent of useSprings:

const [transitions, set] = useTransitions(items, items => items.key, {
  from: {}, enter: {}, leave: {},
});

return (
  <Transition>
    {transitions.map(({ item, key, props }) => (
      <animated.div key={key} style={props}>
        {item.text}
      </animated.div>
    ))}
  </Transition>
);

I feel like splitting useTransition into two things similar to useSpring/useSprings could actually simplify a lot of use cases. I'm not sure how much of that is actually feasible, I just wrote it down the way it makes the most sense to me from a beginners perspective. Hope that helps!

@sami616
Copy link

sami616 commented May 15, 2019

I like the idea of splitting useTransition into two things. Maybe useListTransition and useTransition. The latter for single component mounts and unmounts. As a newbie, using arrays and keys when trying to animate a single component on mount / unmount (ie a modal) was a bit confusing 😄

@drcmda
Copy link
Member Author

drcmda commented May 15, 2019

@biowaffeln

const [props, set] = useSpring({ opacity: 0 })

would set opacity to 0 on every render pass. if the parent decides, for whatever reason to render, opacity will go zero. but this can be fixed using dependencies:

const [props, set] = useSpring({ opacity: visible ? 1 : 0 }, [visible])

And i agree, this should be the only documented api signature.

As for transition and pose's approach. I don't like it. I need things to be explicit without going into too much overhead. But that wrapper is magic again, and it will use React.cloneChildren and mutate stuff. React-spring is shooting for something that gives you control. In this case it costs 1 or 2 lines more, but i think sometimes it's worth it.

@aleclarson the problem is that two api signatures cause unneeded complexity. We should use one signature and perhaps allow users to still use functions, but only to avoid object recreating, like useState.

const [props, set] = useSpring(() => ({ opacity: visible ? 1 : 0 }), [visible])

That would remove half of the needed documentation and beginners aren't confronted with two signatures any longer. They just use this one, if they need set or not.

@ghost
Copy link

ghost commented May 17, 2019

Isn't internally generating the keys an option, perhaps using a WeakMap, so the short signature can be used for everything?

@drcmda
Copy link
Member Author

drcmda commented May 18, 2019

@mfarhadpur which signature?

@aleclarson
Copy link
Contributor

@drcmda He's talking about omitting the item => item.key argument.

@mfarhadpur Yeah, that sounds like a great idea! It doesn't even need to be a WeakMap, just a Map will do.

@ghost
Copy link

ghost commented May 18, 2019

@drcmda Yes, I mean the proposed useTransition(items, config) as @aleclarson explained.

@drcmda
Copy link
Member Author

drcmda commented May 20, 2019

That could work!

@phaistonian
Copy link

@drcmda So, it's coming ?:)

@aleclarson aleclarson pinned this issue May 28, 2019
@aleclarson aleclarson unpinned this issue Jun 16, 2019
@drcmda
Copy link
Member Author

drcmda commented Jun 17, 2019

still in consideration. the only worry i have is having yet another major for this. it would simplify the api, since you only need one signature. but people would have to adjust their code once again. there's a pr now that had this working, it looked good to me at least.

@phaistonian
Copy link

@drcmda Checking the arguments passed would enable supporting both "legacy" and new API, no?

@aleclarson
Copy link
Contributor

aleclarson commented Jun 18, 2019

I don't currently need this, but it's nice to have for sure.

Anyone that wants to implement what @phaistonian said is welcome to. @drcmda has already done most of the work. Just follow these commands:

  • git clone https://github.com/react-spring/react-spring.git -b v9-api Adapt to unified v9 api #670
  • git remote add <user> https://github.com/<user>/react-spring.git (replace <user> with your name, and be sure to fork this repo first)
  • git pull --rebase origin v9 (this syncs with the latest v9)
  • Make your changes (the packages/core/src folder is where the hooks exist)
  • git push -u <user> v9-api (then open a pull request on this repo)
  • Be sure you target the v9 branch when opening your PR

@drcmda
Copy link
Member Author

drcmda commented Jun 18, 2019

Checking the arguments passed would enable supporting both "legacy" and new API, no?

Hmm, sure about that? For instance ...

const props = useSpring({ x: 1 })

would now universally be:

const [props /*, set*/] = useSpring({ x: 1 })

The call signature is the same but the result is different.

Not a big deal, but would force people to once again fix their code by wrapping props into brackets. I like the api for sure, and these 2 signatures have brought about unwanted confusion, it's been one of the main criticisms for sure - so i'm all for it. But then again, ... is it good to break it once again after v8? 🤔

@aleclarson
Copy link
Contributor

For v9, we could require the deps array to opt-in to the [props, set, cancel] return signature.

// Still works like v8
const props = useSpring({ x: 1 })

// These are equivalent in v9 (but the latter is deprecated)
const [props, set] = useSpring({ x: 1 }, [])
const [props, set] = useSpring(() => ({ x: 1 }))

@drcmda
Copy link
Member Author

drcmda commented Jun 18, 2019

interesting ... you two saved the day! 😍

@phaistonian
Copy link

Indeed that could work - sorry for being late to this part :)

ps: The API is half of the project; it defines scalability, attraction, and endurance. Not once have I regretted introducing breaking changes in favor of extra "sanity".

@aleclarson
Copy link
Contributor

The new API for useTransition is being worked on here: #750

@pmndrs pmndrs locked and limited conversation to collaborators Jul 11, 2019
@aleclarson
Copy link
Contributor

Now available in v9.0.0-rc.2 #985

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind: request New feature or request that should be actioned
Projects
None yet
Development

No branches or pull requests

6 participants