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

refactor: [M3-7585] - Upgrade to React 18 #10169

Merged
merged 33 commits into from
Feb 15, 2024

Conversation

bnussman-akamai
Copy link
Contributor

@bnussman-akamai bnussman-akamai commented Feb 9, 2024

Description πŸ“

Updates to React 18 πŸ“¦

This PR does everything needed to update to React 18 and the latest React testing library. πŸŽ‰

I really wanted to break up this work into smaller PR, but the lack of backwards compatibility with React Testing made this approach more realistic. πŸ˜–

Main Changes πŸ”

  • Updates many unit tests to await React Testing Library's userEvent calls ⏱️
  • Rewrites a few unit tests that involve Action Menus to be more verbose and straight forward - this relates to the userEvent change πŸ–ŠοΈ
  • Removes jest-axe because we aren't really using it πŸ—‘οΈ (I'm not opposed to using it, I just think we should revisit this and add it if we're gonna actually commit to using it. It was only used on one test)
  • Refactored unit tests that test React hooks because @testing-library/react-hooks no longer exists. You now have to use renderHook from @testing-library/react πŸͺ
  • Uses a yarn resolution to pin jackspeak version to fix cypress / yarn / storybook issue πŸ“Œ (see const stringWidth = require('string-width');Β cypress-io/cypress#27370 (comment))
  • The native <select> no longer allows placeholder as a prop, so you will see changes to account for this ⌨️

How to test πŸ§ͺ

  • Verify all automated testing passes βœ…
  • Verify the app works as expected when running locally and in the preview link
    • We obviously can't all test every little feature, but spend 30 min or so clicking around Cloud Manager and look for bugs πŸ”Ž

As an Author I have considered πŸ€”

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

@bnussman-akamai bnussman-akamai added the Dependencies Pull requests that update a dependency file label Feb 9, 2024
@bnussman-akamai bnussman-akamai self-assigned this Feb 9, 2024
@bnussman-akamai bnussman-akamai marked this pull request as ready for review February 9, 2024 20:21
@bnussman-akamai bnussman-akamai requested review from a team as code owners February 9, 2024 20:21
@bnussman-akamai bnussman-akamai requested review from cliu-akamai, mjac0bs and cpathipa and removed request for a team February 9, 2024 20:21
Copy link

github-actions bot commented Feb 9, 2024

Coverage Report: ❌
Base Coverage: 81.24%
Current Coverage: 81.22%

Copy link
Contributor

@abailly-akamai abailly-akamai 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 this major update & cleanup (and all the work preceding it)!

I spent quite a bit of time navigating and all over the APP and so far sop good (I will spend more time and we'll have plenty time to test this in develop).

The only thing I noticed is that cold loading behaves a bit differently, seems to take a bit more time to resolve API requests maybe, or for the update to be reflected in the UI?). It often results in visible layout shifts. I can't quite put my finger on it yet. (will spend more time on it and try while serving a production build). It may also just be my own distorted impression.

Meanwhile the build job seems much faster πŸŽ‰

// from the upload page.
flushSync(() => {
dispatchAction(setPendingUpload(false));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is new to me. Could we use const { rerender } = renderWithTheme instead? Not too important, this just looks odd

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is actual application code, so we can't do that. Maybe someone who is more Reduxy and has experience with this uploader could suggest an alternative to this?

My long term hope is that this will all be refactored out of Redux and there will be less sketchy stuff like this

Copy link
Contributor

@mjac0bs mjac0bs 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 completing this effort! πŸŽ‰

The amount of times you had to type/find-and-replace await in this PR is... so many. πŸ˜… πŸ₯‡

I spent a while going through the app, checking functionality of several product create flows (linodes, kube, firewall, volume, obj) and details pages, plus the accounts pages, checking a variety of different components and comparing with prod. Not seeing any regressions and I couldn't seem to replicate what Alban was seeing with longer API req resolution times, though we should keep an eye on that in develop as a team.

If we plan on merging this right after the next release branch is cut, we'll have a good amount of time for this to sit in develop.

@bnussman-akamai bnussman-akamai added the Approved Multiple approvals and ready to merge! label Feb 15, 2024
Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Nice work. Thank you @bnussman! Over all LGTM! No regression found in core features
(Linode / Volume / OBJ / Kubernetes / VPC and firewalls).

@bnussman-akamai bnussman-akamai merged commit a099b8e into linode:develop Feb 15, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Dependencies Pull requests that update a dependency file Ready for Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants