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

Upgrade to React 18 #1759

Merged
merged 6 commits into from
Oct 13, 2022
Merged

Upgrade to React 18 #1759

merged 6 commits into from
Oct 13, 2022

Conversation

lemald
Copy link
Member

@lemald lemald commented Oct 12, 2022

Asana ticket: ⚙️ Upgrade to React 18

Unfortunately these changes are very heavily front-loaded into the first commit. However, that commit description does include a breakdown of what changes are included. The following commits are smaller but should be better broken down.

* Updated React, React Leaflet, and associated packages
* Call createRoot instead of ReactDOM.render
* Updates to match changes to types in React 18
* Removed ref workaround in Map component
* Added babel-jest to transpile some new React Leaflet code that uses
  ES modules
* Removed React Hooks Testing Library, moved to React Testing
  Library's renderHook API.
* Moved a bunch of tests from react-test-renderer to RTL to resolve
  issues
The leaflet.fullscreen package includes its own implementation of
screenfull, but that ends up causing issues in tests and I can't
figure out how to properly get it to find its own implementation of
screenfull, or mock my own. As a result, I have added screenfull as a
separate dependency but for dev only to make the tests work.
expect(tree).toMatchSnapshot()
})

test("renders with vehicles and selected vehicles", () => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: I've removed this (and I think a couple of other places that test rendering selected vehicles) because #1748 moved the rendering of the VPP outside of the page component.

@github-actions
Copy link

Coverage of commit f1f522a

Summary coverage rate:
  lines......: 94.0% (2463 of 2620 lines)
  functions..: 73.0% (1034 of 1417 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@lemald lemald temporarily deployed to dev-blue October 12, 2022 20:50 Inactive
@lemald
Copy link
Member Author

lemald commented Oct 12, 2022

Also note: I think the test "Shuttle Map Page renders with shapes selected" is actually broken (not just with the upgrade, the original snapshot doesn't look like it demonstrates what the name of the test says it's demonstrating). I'll see if I can fix that as part of this PR, or otherwise create a followup ticket.

@lemald
Copy link
Member Author

lemald commented Oct 12, 2022

More generally, I'm going to give this another pass (probably tomorrow) and see if any of the snapshot tests can easily be reworked into more traditional React Testing Library tests so that we don't have to review those huge diffs anymore. I don't want to spend too much time trying to remove snapshot tests as part of this particular PR though just because it's already got a lot going on.

@lemald lemald requested a review from KaylaBrady October 13, 2022 12:54
@lemald lemald marked this pull request as ready for review October 13, 2022 12:56
@github-actions
Copy link

Coverage of commit b5b0c07

Summary coverage rate:
  lines......: 94.0% (2463 of 2620 lines)
  functions..: 73.0% (1034 of 1417 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

Copy link
Contributor

@KaylaBrady KaylaBrady left a comment

Choose a reason for hiding this comment

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

Looks good! Only blocking comment is about the recenter control

assets/tests/hooks/useNotifications.test.tsx Show resolved Hide resolved
.github/workflows/report-coverage.yml Show resolved Hide resolved
assets/src/components/map.tsx Outdated Show resolved Hide resolved
assets/package.json Show resolved Hide resolved
assets/src/components/map.tsx Show resolved Hide resolved
@lemald lemald force-pushed the lem-react-18 branch 2 times, most recently from c65ccbb to 64f8075 Compare October 13, 2022 15:45
@github-actions
Copy link

Coverage of commit 64f8075

Summary coverage rate:
  lines......: 94.0% (2463 of 2620 lines)
  functions..: 73.0% (1034 of 1417 functions)
  branches...: no data found

Files changed coverage rate: n/a

Download coverage report

@lemald lemald temporarily deployed to dev-blue October 13, 2022 15:53 Inactive
Copy link
Contributor

@KaylaBrady KaylaBrady left a comment

Choose a reason for hiding this comment

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

Looks good!

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