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

deps: upgrade react-spring to v9 #1218

Closed
wants to merge 5 commits into from

Conversation

hsunpei
Copy link
Contributor

@hsunpei hsunpei commented May 15, 2021

🏠 Internal

@visx/react-spring is currently dependent on react-spring v8.
While react-spring introduces several breaking changes in v9, and thus making @visx/react-spring incompatible with projects using the latest version of react-spring.

This PR upgrades the react-spring to v9.

Note

There are still some typing issues in the formal release of v9 which prevents inferring the type of useTransition correctly.
To avoid this issue, this PR uses 9.0.0-rc.3 as the peer dependency instead of 9.1.2.

Copy link
Collaborator

@williaster williaster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @garfieldduck thanks so much for the contribution! I had a couple super minor suggestions, and will also try to pull the branch locally since our visual diffing in CI doesn't capture animation. Otherwise this lgtm!

@@ -0,0 +1,7675 @@
# THIS IS AN AUTOGENERATED FILE. DO NOT EDIT THIS FILE DIRECTLY.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since visx uses yarn workspaces, we have only committed the yarn.lock file in the repo root directory. so I think if you remove this file we can just rely on the updated root yarn.lock you've updated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, definitely! I didn't notice that I added this file accidentally.

animateXOrY: horizontal ? 'x' : 'y',
animationTrajectory,
}),
keys: (tick: { value: any }) => `${tick.value}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wonder if we could do something like this instead of using any

// add this to the import above
import { ComputedTick, ... } from '@visx/axis/lib/types';

// in hook
keys: (tick: ComputedTick<Scale>) => `${tick.value}`,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great point! I'll change it.

) => {
const tickLabelProps = allTickLabelProps[index] ?? allTickLabelProps[0] ?? {};
return item == null || key == null ? null : (
{animatedTicks(({ fromX, toX, fromY, toY, opacity }, item, { key }, index) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: im getting a ts warning -> Property 'fromX' does not exist on type '{}'.ts(2339) for { fromX, toX, fromY, toY, opacity },
might be good to keep the @ts-ignore if no explicit type is defined

// @ts-ignore react-spring types only include CSSProperties

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be caused by the type inference issue of 9.1.2.
After running yarn, it picks up9.1.2 automatically instead of 9.0.0-rc.3.
But I think it's not nice to stick to 9.0.0-rc.3 instead of using ^9.0.0-rc.3 in peer dependencies, so I think putting @ts-ignore here is indeed a good idea.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like this is still broken in 9.2.0 so maybe @ts-ignore is the way to go 😢

@williaster
Copy link
Collaborator

Hey @garfieldduck sorry for the delay on this PR, we've been trying to figure out how to stage this alongside the react-17 peerDependency update. We've started in on that (#1268) and are thinking we should try to land this first because this discussion suggests that react@17 support was added in react-spring@9.2.0.

Do you want to try bumping up to ^9.2.0 then we can try and get this merged / get a @visx/*@2.0.0-alpha out to build off of in the react-17 work? If you don't have bandwidth in the next couple of days I can help try to bump to the latest and get it in. Thanks again for this BIG contribution! 🙏

@williaster
Copy link
Collaborator

Superseded by #1277

@williaster williaster closed this Jul 21, 2021
@hsunpei
Copy link
Contributor Author

hsunpei commented Jul 25, 2021

Hey @williaster, thanks so much for the upgrade 🎉!!!
Sorry I was on vacation and did not notice your mention.

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

Successfully merging this pull request may close these issues.

3 participants