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

Migration to react 18 #727

Merged
merged 44 commits into from
Jul 27, 2023
Merged

Migration to react 18 #727

merged 44 commits into from
Jul 27, 2023

Conversation

Angatupyry
Copy link
Collaborator

What's new

React 18.

What changed?

Many dependencies had to change due to incompatibility with React 18.
The most relevant change was the routing handling.
It was upgraded to react router 6.

The most relevant change was to add /* to the paths that have children, in our case, only /admin in order to render the child paths.

Error before adding the /* path

You rendered descendant <Routes (or called `useRoutes()`) at "/" (under <Route path="/">) 
but the parent route path has no trailing "*". This means if you navigate deeper, 
the parent won't match anymore and therefore the child routes will never render.

Please change the parent <Route path="/"> to <Route path="*">.

You can test for this error by removing /* from line 17 of the file /util/url.ts of the dashboard package

The other changes are react router v6 enhancements or different package names that serve the same purpose as the previous version. You can look at some of them here

Other dependencies changed were.

  • testing-library/dom
  • testing-library/jest-dom
  • testing-library/react
  • testing-library/user-event

Many test files were changed due to incompatibility with react 18.

Some of the most relevant changes were:

Obs: In react-component it was not possible to upgrade to the latest testing library version due to incompatibility with webpack and karma. I consider something not relevant to address now. Therefore:

  • This code was added to avoid the alert
Warning: An update to ComponentName inside a test was not wrapped in act(...).
ocasionada por incompatibilidad de webpack y testing library. 

const originalError = console.error;
beforeAll(() => {
  // this is just a little hack to silence
  // Warning: An update to ComponentName inside a test was not wrapped in act(...).
  console.error = (...args) => {
    if (/Warning.*not wrapped in act/.test(args[0])) {
      return;
    }
    originalError.call(console, ...args);
  };
});
  • Use fireEvent instead of userEvent in many cases because by default fireEvent is wrapped inside act function and this is useful when the user does some action and this action will cause component updates and re-render. (https://kentcdodds.com/blog/common-mistakes-with-react-testing-library)

  • This is a relatively major change, so please test all possible cases, (create tasks, create alerts, create schedules). Especially the interactions of the routes in admin.

Self-checks

  • I have prototyped this new feature (if necessary) on Figma
  • I'm familiar with and follow this Typescript guideline
  • I added unit-tests for new components
  • I tried testing edge cases
  • I tested the behavior of the components that interact with the backend, with an e2e test

Discussion

Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
…serEvent

Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
…library

Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
@codecov
Copy link

codecov bot commented Jul 13, 2023

Codecov Report

Merging #727 (8e62a77) into main (4634cc5) will decrease coverage by 22.96%.
Report is 2 commits behind head on main.
The diff coverage is 60.00%.

@@             Coverage Diff             @@
##             main     #727       +/-   ##
===========================================
- Coverage   56.30%   33.35%   -22.96%     
===========================================
  Files         293      131      -162     
  Lines        6981     3676     -3305     
  Branches      943      836      -107     
===========================================
- Hits         3931     1226     -2705     
+ Misses       2858     2312      -546     
+ Partials      192      138       -54     
Flag Coverage Δ
api-server ?
dashboard 18.59% <61.11%> (+0.06%) ⬆️
react-components 51.61% <50.00%> (-6.36%) ⬇️
rmf-auth ∅ <ø> (∅)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
packages/dashboard/src/components/admin/router.tsx 0.00% <ø> (ø)
...shboard/src/components/admin/user-profile-page.tsx 0.00% <0.00%> (ø)
packages/dashboard/src/components/app.tsx 0.00% <0.00%> (ø)
packages/react-components/lib/map/with-label.tsx 8.33% <0.00%> (ø)
packages/dashboard/src/components/admin/drawer.tsx 88.88% <60.00%> (-5.56%) ⬇️
.../dashboard/src/components/admin/user-list-card.tsx 76.56% <100.00%> (+0.80%) ⬆️
packages/dashboard/src/components/appbar.tsx 32.03% <100.00%> (+1.41%) ⬆️
packages/dashboard/src/util/url.ts 90.00% <100.00%> (ø)
packages/react-components/lib/locale.tsx 16.66% <100.00%> (ø)

... and 132 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Signed-off-by: angatupyry <fierrofenix@gmail.com>
Signed-off-by: angatupyry <fierrofenix@gmail.com>
Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

Thanks for the enormous effort with the migration! While testing, I started changing a number of things and thought it would be best if I just branched off your work with my suggesions, #729. let me know what you think.

Other than the items I listed in #729, LGTM!

* Adding link to unresolved issue to testing

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Added back missing test

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Reverting stack navigator tests

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Remove the rest of reporting components from react-components

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Remove deprecated command forms

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Revert to remove expectation of loading text since that is not what we are testing

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

* Add DashboardRoute to the route map since it returns undefined anyway

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>

---------

Signed-off-by: Aaron Chong <aaronchongth@gmail.com>
Copy link
Member

@aaronchongth aaronchongth left a comment

Choose a reason for hiding this comment

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

LGTM!

@aaronchongth aaronchongth merged commit 22355c1 into main Jul 27, 2023
5 checks passed
@aaronchongth aaronchongth deleted the cr/react-18 branch July 27, 2023 02:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants