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

feat: update react & react-dom to v17 #1009

Merged
merged 60 commits into from
Jan 5, 2024

Conversation

Mashal-m
Copy link
Contributor

@Mashal-m Mashal-m commented Jul 24, 2023

Ticket

Upgrade React JS to v17

Description

  • Updated react & react-dom to v17 along with react-test-renderer, react-helmet, react-textarea-autosize, @testing-library/react and react-redux to respective compatible versions
  • Since Enzyme does not provide support React 17 (enzymejs/enzyme#2429) or 18 (enzymejs/enzyme#2524) itself we switched to community-supported project that aim to provide compatibility between Enzyme and React 17, replacing enzyme-adapter-react-16 with @wojtekmaj/enzyme-adapter-react-17
  • Replaced react-truncate with react-lines-ellipsis due to dependency issues with react v17
  • Update edx packages @edx/frontend-enterprise-catalog-search, @edx/frontend-enterprise-hotjar , @edx/frontend-enterprise-logistration @edx/frontend-enterprise-utils @edx/frontend-platform, and @edx/paragon

For all changes

  • Ensure adequate tests are in place (or reviewed existing tests cover changes)

Only if submitting a visual change

  • Ensure to attach screenshots
  • Ensure to have UX team confirm screenshots

@codecov
Copy link

codecov bot commented Aug 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (dbdbe10) 85.42% compared to head (0851d45) 85.42%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1009   +/-   ##
=======================================
  Coverage   85.42%   85.42%           
=======================================
  Files         494      494           
  Lines       10604    10604           
  Branches     2214     2214           
=======================================
  Hits         9058     9058           
  Misses       1504     1504           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Mashal-m Mashal-m marked this pull request as ready for review August 11, 2023 10:03
package.json Outdated Show resolved Hide resolved
Comment on lines 159 to 161
/* Upgrading the @edx/paragon version from 20.41.0 to 20.42.0
caused this function to be called twice. Setting this to 2 in order to fix the test */
expect(sendEnterpriseTrackEvent).toHaveBeenCalledTimes(2);
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, this doesn't feel quite like a "fix"... If the component itself is calling sendEnterpriseTrackEvent more than expected when the use toggled between these radio options (see screenshot), then that is a bug as it would skew the product analytics around this event (duplicate events for a single click).

Can this be reproduced on the Paragon component itself (i.e., onChange callback function passed to Form.Radioset)? If not, then it might be an issue with this repo's usage of the Paragon component.

image

Copy link
Contributor Author

@Mashal-m Mashal-m Aug 24, 2023

Choose a reason for hiding this comment

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

I've implemented the Form.Radioset component in another part of my code, and I'm facing an issue where the onChange function is getting called twice.

Attaching the code and UI snippet below.
Screenshot 2023-08-24 at 6 23 11 PM
262987009-08f997ac-7b60-4004-a5b3-c381abd5b32f

Copy link
Member

Choose a reason for hiding this comment

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

@Mashal-m I believe this is related to this Paragon bug and associated fix (in code review), and will apply to Paragon v21.

This same issue came up recently when trying to upgrade Paragon to latest v20 as well. We opted to defer on the Paragon latest v20 upgrade as this regression ends up causing duplicate success Toast messages to appear within some of our features in this MFE...

[question] Is it possible to move forward with this PR without upgrading Paragon to latest v20 to avoid introducing this regression?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm start is failing on the master branch, and I'm encountering the following error (attached screenshot). The fix for this error was released in v20.43.3. However, v20.42.0 is causing this test failure, so to resolve the master issue, we need to upgrade the Paragon version.
Screenshot 2023-10-13 at 12 14 44 PM

Copy link
Member

Choose a reason for hiding this comment

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

Ah, got it. Thanks for clarifying. Luckily, the aforementioned bug preventing us from upgrading to latest v20, causing the issue with the duplicate onChange calls was fixed and backported to v20.46.3 earlier today.

We should be able to move forward with upgrading to latest v20 at this point, without needing to update tests regarding expect(sendEnterpriseTrackEvent).toHaveBeenCalledTimes(2); 🤞

@@ -242,38 +242,40 @@ describe('Test authorization flows for Blackboard and Canvas', () => {
expect(mockCanvasFetch).toHaveBeenCalledWith(1);
});
test('Canvas config is activated after last step', async () => {
renderWithRouter(<SettingsCanvasWrapper />);
mockCanvasUpdate.mockResolvedValue({ data: mockCanvasResponseDataActive });
setTimeout(async () => {
Copy link
Member

Choose a reason for hiding this comment

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

[curious] Why is a setTimeout necessary here? It feels like a code smell that setTimeout would be needed in a test like this when its used React Testing Library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The initial issue was I ran into a "Null: create event" error when running this test file. The problem seemed to be with the last section. Async/await operations weren't working properly there, so I had to use setTimeout to add a small delay in order to clear the current stack.

I have made some changes for this particular test case (commit)

@Mashal-m Mashal-m force-pushed the mashal-m/react-upgrade-to-v17 branch from 9d960b2 to d8a0b20 Compare August 24, 2023 05:56
@Mashal-m
Copy link
Contributor Author

Left some additional minor, non-blocking comments. Once Paragon upgrades to v20.46.3 to pull in the fix for the duplicate onChange in a couple Paragon components, this PR should be good to go.

I have pushed the requested changes and upgraded Paragon as well, we are good to go!

@Mashal-m Mashal-m self-assigned this Dec 1, 2023
@Mashal-m Mashal-m force-pushed the mashal-m/react-upgrade-to-v17 branch from 66c3da9 to f769c39 Compare January 3, 2024 09:35
Copy link
Member

@brobro10000 brobro10000 left a comment

Choose a reason for hiding this comment

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

I did notice some failing tests in the PR.
For the EmailAddressTableCell.tests.jsx failing tests, refactoring to the await waitFor syntax seemed to fix the issue with the enterprise track event count and could be used to fix up similar issues the tests are showing

    await waitFor(() => expect(screen.findByText('Learner data disabled', { exact: false })));
    await waitFor(() => expect(sendEnterpriseTrackEvent).toHaveBeenCalledTimes(1));

Great job refactoring to paragons Truncate component 👍🏽

package.json Outdated
"react-helmet": "5.2.1",
"react": "17.0.2",
"react-dom": "17.0.2",
"react-helmet": "^6.1.0",
Copy link
Member

Choose a reason for hiding this comment

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

[nit] I would pin this upgrade 👍🏽

@Mashal-m
Copy link
Contributor Author

Mashal-m commented Jan 4, 2024

I did notice some failing tests in the PR. For the EmailAddressTableCell.tests.jsx failing tests, refactoring to the await waitFor syntax seemed to fix the issue with the enterprise track event count and could be used to fix up similar issues the tests are showing

    await waitFor(() => expect(screen.findByText('Learner data disabled', { exact: false })));
    await waitFor(() => expect(sendEnterpriseTrackEvent).toHaveBeenCalledTimes(1));

Great job refactoring to paragons Truncate component 👍🏽

Fixed the tests, it is ready for merge now.

@brobro10000 brobro10000 merged commit 1797209 into master Jan 5, 2024
6 checks passed
@brobro10000 brobro10000 deleted the mashal-m/react-upgrade-to-v17 branch January 5, 2024 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants