Add onRest to TransitionMotion

[ollwenjones]

Implements a simple onRest prop for TransitionMotion using the existing shouldStopAnimationAll helper function.

[ollwenjones]

I had trouble building this locally, perhaps something with the rollup config and the fact that I am on a Windows box?

./src/react-motion.js → dist/react-motion.esm.js...
[!] (size-snapshot plugin) Error: Error transforming bundle with 'size-snapshot' plugin: ModuleNotFoundError: Module not found: Error: Can't resolve './Motion.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './StaggeredMotion.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './TransitionMotion.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './presets.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './reorderKeys.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './spring.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './stripStyle.js' in '/'
Error: Error transforming bundle with 'size-snapshot' plugin: ModuleNotFoundError: Module not found: Error: Can't resolve './Motion.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './StaggeredMotion.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './TransitionMotion.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './presets.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './reorderKeys.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './spring.js' in '/'
ModuleNotFoundError: Module not found: Error: Can't resolve './stripStyle.js' in '/'
    at error (C:\devel\repos\react-motion\node_modules\rollup\dist\rollup.js:3356:15)
    at C:\devel\repos\react-motion\node_modules\rollup\dist\rollup.js:13582:17
    at <anonymous>

[nkbt]

Yeah, ./ path is for linux/macos only. You can use bash/linux subsystem in win10 natively, works well.

[ollwenjones]

IIRC windows treats / as the root of the drive, and the . is necessary in ./ to ensure it's treated as relative... it seems really specific to roll-up in this case, because the webpack build is fine. 🤔 Also the first two roll-up build steps are fine. - I've never had a problem with relative paths with a ./ on windows before, so I think it's that it's looking in the wrong directory - relative to the wrong place - probably C:\

[ollwenjones]

I think it would be a lot better to add some config to ensure the build is running from the right directory, rather than ask windows-based contributors to re-configure their entire setup inside the embedded linux subsystem in win10. When I investigated that, it didn't share much with the native OS file system, so it would take re-configuring almost everything about the development environment (node, git, etc.)

[nkbt]

I do not do any dev on windows (except on "bash on windows", which is more just for fun then practical, since I have macbook sitting right next to my gaming windows desktop) and do not know any other contributors working on native windows, so it is kind of impossible to support something you do not work on daily =(

I used to fully support windows dev process on my libs, but it was quite a lot of pain and extra work. And build was more complicated as well + one xtra CI (appveyor), which adds time to build and you need to keep updating and supporting it too. And it was not some kind of work I enjoyed doing. At some point I've decided it is not worth it as soon as linux subsystem on windows was released.

[ollwenjones]

That's understandable. In this particular case it is really only the naked / that is the problem. I was thinking it should be something as simple as a setting a baseDir either in Babel or Rollup, but neither seem to expose that in a straightforward way without plugins. I can try to contribute something to this, but I would see that being part of it's own PR, instead of dirtying up this one.

I tested these code changes in the demos (webpack) and added a unit test case for it, so I'm pretty confident these code changes are ready to :shipit: unless there's some disagreement with the approach. The windows dev environment problem just prevents me doing a build to port into the project where I need the change until a new version is released.

[ollwenjones]

looks like all the builds work here on windows except the esm one... I can probably work with that for progressing on my task. We have a little time this sprint, so it's not super urgent, but it would save some time & rework if this got merged ahead of sprint's end 😆 & make the windows build setup less urgent for me.

[ollwenjones]

Had some time so I tried the linux-inside-windows and it built no problem.

[ollwenjones]

Any way to get movement here?

[Sutikshan]

@ollwenjones I hope, you tested this at your end. we too need this, hopefully we can see this merged soon. Thanks.

[ollwenjones]

@Sutikshan absolutely. I published it to our local npm repo to get a bug-fix delivered. It's "in the wild" from my point of view. I'm not sure what the hesitation is for merging it this point.

[remyoudemans]

Can this be merged? This is valuable.

[ollwenjones]

Is this waiting for me to fix conflicts?