-
Notifications
You must be signed in to change notification settings - Fork 94
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
test: run test with react v16 v17 and v18 #392
test: run test with react v16 v17 and v18 #392
Conversation
100terres
commented
Aug 20, 2022
•
edited
Loading
edited
- fix Support strict mode with react v18 #317
- fix Run test suite for react v16, v17 and v18 #320
useEffect(() => { | ||
// clean up the registry to avoid any leaks | ||
return function unmount() { | ||
// clean up the registry to avoid any leaks | ||
// doing it after an animation frame so that other things unmounting | ||
// can continue to interact with the registry | ||
requestAnimationFrame(registry.clean); | ||
// FIXME: we do not know if this is still needed, but to make sure we do not | ||
// break any existing existing code using react 16 and 17 we'll | ||
// continue to clean up after an animation frame | ||
// | ||
// The requestAnimationFrame polyfill was added in this commit: | ||
// https://github.com/atlassian/react-beautiful-dnd/pull/1487/commits/8bdffb9d077b0009400620d9cf6575bba7af13dc#diff-b3b2de485fa432e394aebc8abf54be40ad7fac9b39a2ed818fddfd56f1786c53 | ||
if (React.version.startsWith('16') || React.version.startsWith('17')) { | ||
// doing it after an animation frame so that other things unmounting | ||
// can continue to interact with the registry | ||
requestAnimationFrame(registry.clean); | ||
} else { | ||
// starting with react v18, we must invoke clean immediately | ||
// we won't be able to access the registry after the unmount | ||
// more details here: https://beta.reactjs.org/learn/synchronizing-with-effects#how-to-handle-the-effect-firing-twice-in-development | ||
registry.clean(); | ||
} | ||
}; | ||
}, [registry]); |
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.
ℹ️ Usage without the useEffectOnce
as we do in hello-pangea/dnd.
c13f7f7
to
c17701b
Compare
5768ade
to
36e97eb
Compare
e2c13c9
to
36e97eb
Compare
@Xhale1 this PR is ready for your review 🙂 I've invited you as a maintainer. I'll be able to assign you as a reviewer once you've accept the invitation. |
- test-unit: | ||
matrix: | ||
parameters: | ||
react-major-version: ['16', '17', '18'] |
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 is to run our unit tests three times with a different value in the REACT_MAJOR_VERSION
environment variable.
reactOptions: { | ||
strictMode: true, | ||
}, |
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.
ℹ️ strict mode enabled in storybook 🎉
export const decorators: DecoratorFn[] = [ | ||
(story, { id }: { id: string }) => { | ||
resetData(id); | ||
|
||
return <Story key={id} />; | ||
return story({ key: id }); | ||
}, | ||
(Story: React.ElementType): React.ReactElement => ( | ||
<GlobalStyles> | ||
<Story /> | ||
</GlobalStyles> | ||
), | ||
withPerformance, | ||
(story) => <GlobalStyles>{story()}</GlobalStyles>, | ||
]; |
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.
ℹ️ There was an issue with the strict mode, because of we were using the story
as a component.
if (['16', '17'].includes(reactMajorVersion)) { | ||
config.testPathIgnorePatterns = [ | ||
...(config.testPathIgnorePatterns || []), | ||
// These test do not requires react and will | ||
// be run in the base run (with react v18) | ||
'test/unit/docs', | ||
'test/unit/health', | ||
]; | ||
config.cacheDirectory = `.cache/jest-cache-react-${reactMajorVersion}`; | ||
config.moduleNameMapper = { | ||
'^@testing-library/react((\\/.*)?)$': `@testing-library/react-16-17$1`, | ||
'^react-dom((\\/.*)?)$': `react-dom-${reactMajorVersion}$1`, | ||
'^react((\\/.*)?)$': `react-${reactMajorVersion}$1`, | ||
}; | ||
} |
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.
ℹ️ Important peice of code to be able to test with react 16, 17 and 18.
|
||
// override react-virtualized's types, because the current version | ||
// uses react 17 types | ||
declare module 'react-virtualized' { |
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.
ℹ️ react-virtualized
does not support react 18
… set (#392) also reactor how we manipulate environment variables in our test
… set (#392) also reactor how we manipulate environment variables in our test
… set (#392) also reactor how we manipulate environment variables in our test