-
Notifications
You must be signed in to change notification settings - Fork 150
feat: upgrade react to v18 #1220
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1220 +/- ##
==========================================
- Coverage 58.53% 58.50% -0.03%
==========================================
Files 117 117
Lines 2320 2321 +1
Branches 639 641 +2
==========================================
Hits 1358 1358
- Misses 901 902 +1
Partials 61 61 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
6867a1f
to
9134eea
Compare
Thank you so much for opening this PR! The plan for the React 18 upgrades is to use |
@brian-smith-tcril sure, let me update the PR. |
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.
Thank you so much for this! Overall it's looking great. Just one small note about a snapshot test that seems to have broken.
/>, | ||
] | ||
`; | ||
exports[`SuccessModal should match open success modal snapshot 1`] = `undefined`; |
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.
It looks like something broke this snapshot.
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.
@brian-smith-tcril resolved.
0c2a585
to
b05dc8f
Compare
/>, | ||
] | ||
`; | ||
exports[` 1`] = `undefined`; |
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.
@huniafatima-99 it looks like this changed but now instead of replacing the actual snapshot with
exports[`SuccessModal should match open success modal snapshot 1`] = `undefined`;
it doesn't even say "SuccessModal should match open success modal snapshot" anymore
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 did a bit of digging into this. If you replace SuccessModal.test.jsx
with
import React from 'react';
import renderer from 'react-test-renderer';
import { IntlProvider, injectIntl } from '@edx/frontend-platform/i18n';
import { waitFor } from '@testing-library/react';
import { SuccessModal } from './SuccessModal'; // eslint-disable-line import/first
// Modal creates a portal. Overriding createPortal allows portals to be tested in jest.
jest.mock('react-dom', () => ({
...jest.requireActual('react-dom'),
createPortal: jest.fn(node => node), // Mock portal behavior
}));
const IntlSuccessModal = injectIntl(SuccessModal);
describe('SuccessModal', () => {
let props = {};
beforeEach(() => {
props = {
onClose: jest.fn(),
status: null,
};
});
it('should match default closed success modal snapshot', async () => {
await waitFor(() => {
const tree = renderer.create((
<IntlProvider locale="en"><IntlSuccessModal {...props} /></IntlProvider>))
.toJSON();
expect(tree).toMatchSnapshot();
});
await waitFor(() => {
const tree = renderer.create((
<IntlProvider locale="en"><IntlSuccessModal {...props} status="confirming" /></IntlProvider>))
.toJSON();
expect(tree).toMatchSnapshot();
});
await waitFor(() => {
const tree = renderer.create((
<IntlProvider locale="en"><IntlSuccessModal {...props} status="pending" /></IntlProvider>))
.toJSON();
expect(tree).toMatchSnapshot();
});
await waitFor(() => {
const tree = renderer.create((
<IntlProvider locale="en"><IntlSuccessModal {...props} status="failed" /></IntlProvider>))
.toJSON();
expect(tree).toMatchSnapshot();
});
});
it('should match open success modal snapshot', async () => {
await waitFor(() => {
const tree = renderer
.create((
<IntlProvider locale="en">
<IntlSuccessModal
{...props}
status="deleted"
/>
</IntlProvider>
))
.toJSON();
expect(tree).toMatchSnapshot();
});
});
});
it will match the snapshots as they are currently saved on master
.
It does feel a bit odd that we have a bunch of snapshots specifically looking for null
- but I consider cleaning that up as out of scope for this PR. I looked through the history and found those changed in #654, so continuing to check for null
snapshots in this PR is fine.
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.
@brian-smith-tcril You are right, waiting for react and document to be ready before taking the snapshot did the magic. Thank you for the solution. Much appreciated.
Sandbox deployment failed 💥 |
Sandbox deployment successful 🚀 |
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.
LGTM!
Note
The codecov difference is just because index.jsx
is not covered by any tests in this repo. I consider it non-blocking.
Description
This PR upgrades react to v18. It also updated the test cases to remove the deprecated approaches according to react 18.
How Has This Been Tested?
All the test suites were ran and they were successful. In addition, major functionalities are working when ran the MFE in the browser.
Merge Checklist
Post-merge Checklist