-
Notifications
You must be signed in to change notification settings - Fork 721
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 to react v17 and react-testing-library #1268
Conversation
|
||
expect(Element.prototype.getBoundingClientRect).toHaveBeenCalled(); | ||
expect(RenderedComponent.prop('rect')).toEqual(expectedRectShape); | ||
expect(RenderedComponent.prop('parentRect')).toEqual(expectedRectShape); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@williaster This is where RTL's philosophy of testing differs from Enzyme. Directly checking internal props & states is a NO-NO in RTL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^I agree the RTL philosophy is better for things like this 💯 this is why enzyme struggles with adapters for each react release 😅
import { GridPolar, GridAngle, GridRadial } from '../src'; | ||
import { scaleLinear } from '../../visx-scale'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this will cause tests to fail when visx-scale in the upper folder is not built
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch. this used to happen in several packages and was mostly cleaned up in the TS re-write but clearly missed some 🙉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! this looks great to me, I think I only had one real suggestion which was to set @visx/demo
to ^17.0.0
.
For the react-spring@9
issues, do you think we should merge that in and then rebase? Looks like it has a few remaining issues but I'll try to get it to pass CI today.
expect(RenderedComponent.prop('parentRect')).toEqual(expectedRectShape); | ||
expect(typeof RenderedComponent.prop('getRects')).toBe('function'); | ||
const RenderedComponent = getByTestId('BoundingRectsComponent'); | ||
const RenderedComponentParent = getByTestId('BoundingRectsComponentParent'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TIL getByTestId
packages/visx-demo/package.json
Outdated
@@ -87,7 +87,7 @@ | |||
"nprogress": "^0.2.0", | |||
"prismjs": "^1.19.0", | |||
"raw-loader": "^4.0.0", | |||
"react": "^16.9.0", | |||
"react": "^16.9.0 || ^17.0.0-0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since this one is a dependency
(and not really intended to be installed) we could probably just use ^17.0.0
here? this would also let us test and make sure everything works with 17
in the demo site.
import { GridPolar, GridAngle, GridRadial } from '../src'; | ||
import { scaleLinear } from '../../visx-scale'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great catch. this used to happen in several packages and was mostly cleaned up in the TS re-write but clearly missed some 🙉
const HOC = withParentSize(Component); | ||
const wrapper = mount(<HOC />); | ||
const { getByTestId } = render(<HOC initialWidth={200} initialHeight={200} />); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice way to avoid getBoundingClientRect
, I think when we initially wrote this test it didn't take the initialWidth/Height
import { Arc } from '../src'; | ||
import { ArcProps } from '../src/shapes/Arc'; | ||
import '@testing-library/jest-dom'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure if this will throw a lint error for being after local imports 🤔 fine if it doesn't throw!
</svg>, | ||
); | ||
const PathElement = container.querySelector('path'); | ||
expect(fakeRef.current).toContainElement(PathElement); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also TIL about the @testing-library/jest-dom
selectors, nice 😎
ah, one other thing we should bump is the |
… Tooltip, withRegisteredData
visx-xychart has a huge amount of tests using mount and checking on implementation details with enzyme. I opened a separate PR that merges into this branch |
0611d48
to
7c8f061
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woot! 🔥 This is awesome, thanks for doing such a great and thorough job. There are a lot of packages, and going through them all is big task, so nicely done 🙌
I had one small remaining comment but otherwise think we could get this in! (we could also address that comment in a small followup / it's very minor)
As far as releases, there may be one more breaking change in #1305 and #1121 . So I think we should do another 2.0.0-alpha
pre-release (I think we need a patch-release
to force a change based on how our CI works) and then merge those two and graduate to 2.0.0
asap.
@@ -1,6 +1,8 @@ | |||
import React from 'react'; | |||
import { shallow, mount } from 'enzyme'; | |||
import { shallow } from 'enzyme'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't comment on it since it wasn't changed, but wonder if we should just remove the two shallow
tests here since they only assert that there are not errors. then we'd be fully on RTL as well 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea sure, these tests are trivial. For later iterations, I would like to convert the shallow tests to RTL as well (these are relatively easier, most of them are just renaming work), so we can fully remove enzyme out of a devDependencies.
🎉 This PR is included in version |
🎉 This PR is included in version |
🎉 This PR is included in version |
🎉 This PR is included in version |
🎉 This PR is included in version |
💥 Breaking Changes
🚀 Enhancements
📝 Documentation
Tests to fix:
🐛 Bug Fix
🏠 Internal