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

Chore: Upgrade to react 18 #64428

Merged
merged 69 commits into from Apr 11, 2023
Merged

Chore: Upgrade to react 18 #64428

merged 69 commits into from Apr 11, 2023

Conversation

ashharrison90
Copy link
Contributor

@ashharrison90 ashharrison90 commented Mar 8, 2023

What is this feature?

  • bumps react, react-dom, @testing-library/dom, @testing-library/react and react-test-renderer to latest versions supporting react 18
  • moves to the new rendering api
  • removes <StrictMode> wrapping around the whole app.
    • StrictMode is much stricter in react 18.
    • we will need to gradually reenable it and fix issues as we go.
    • there are some libraries we use which need updating to work with the new strict mode as well
  • removes testing-library/react-hooks and replaces with the same renderHook method now exposed in @testing-library/react, as recommended in the migration guide
  • patches @storybook/addon-docs and react-split-pane to add the children prop to some components that were using React.FC internally.
    • these can be removed once the fixes are fed back into these packages.
  • other misc type fixes due to react 18 being better at inferring types
    • one to call out is that renderOrCallToRender now requires props be passed when using the function variety. even if those props are {}.
  • rewrites DashboardPage.test.tsx/PublicDashboardPage.test.tsx to be structured more like a regular RTL test.
    • these still test exactly the same things.
    • i promise i tried to keep the structure intact, but i couldn't find a way without them causing act warnings (cc @torkelo , i know you like the previous style, maybe you can find a way 🤞 ).
    • i suspect jest/react is doing some magic to prevent act warnings that doesn't play nicely with the structure these tests used.
  • other misc test fixes

Why do we need this feature?

  • take advantage of new react features + improvements

Who is this feature for?

  • everyone

Which issue(s) does this PR fix?:

Fixes #

Special notes for your reviewer:

Release notice breaking change

Grafana has been upgraded to React 18 and now leverages the new React client rendering API. Plugin authors in particular should be aware, as there could be unintended side effects due to the changes around automatic batching of state updates and consistent useEffect timings. Be sure to test your plugins and reference the React 18 upgrade docs here: https://react.dev/blog/2022/03/08/react-18-upgrade-guide

@grafanabot grafanabot added datasource/Azure Azure Monitor Datasource datasource/Tempo labels Mar 13, 2023
@@ -114,10 +114,9 @@
"@rtsao/plugin-proposal-class-properties": "7.0.1-patch.1",
"@swc/core": "1.3.38",
"@swc/helpers": "0.4.14",
"@testing-library/dom": "8.20.0",
"@testing-library/dom": "9.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

RTL is built on top testing-library/dom, so should include all its functions. I quickly checked our codebase and it seems like we use testing-library/dom mostly for the within function, which is also available from testing-library/react. Should we try to remove this package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok so i've changed the instances of the code that are using within/fireEvent from @testing-library/dom directly to instead use the methods from @testing-library/react, however i don't think we can remove the package itself as @testing-library/user-event declares a peer dependency on it.

Copy link
Contributor

@Clarity-89 Clarity-89 Apr 4, 2023

Choose a reason for hiding this comment

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

I think @testing-library/react should pull it in as a dependency? Also found an issue where it says that @testing-library/dom shouldn't be listed as dependency unless used directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it does, but that will still leave us with warnings when yarn installing:

➤ YN0002: │ grafana@workspace:. doesn't provide @testing-library/dom (p15dc6), requested by @testing-library/user-event

there's another issue here saying that installing it directly is the correct approach when using @testing-library/user-event.

Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@joshhunt joshhunt left a comment

Choose a reason for hiding this comment

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

Lets do this!

@ashharrison90 ashharrison90 merged commit 1261345 into main Apr 11, 2023
17 checks passed
@ashharrison90 ashharrison90 deleted the ash/react-18 branch April 11, 2023 09:51
alexmobo pushed a commit that referenced this pull request Apr 14, 2023
* update react 18 related deps

* fix some types

* make sure we're on react-router-dom >= 5.3.3

* Use new root API

* Remove StrictMode for now - react 18 double rendering causes issues

* fix + ignore some @grafana/ui types

* fix some more types

* use renderHook from @testing-library/react in almost all cases

* fix storybook types

* rewrite useDashboardSave to not use useEffect

* make props optional

* only render if props are provided

* add correct type for useCallback

* make resourcepicker tests more robust

* fix ModalManager rendering

* fix some more unit tests

* store the click coordinates in a ref as setState is NOT synchronous

* fix remaining e2e tests

* rewrite dashboardpage tests to avoid act warnings

* undo lint ignores

* fix ExpanderCell types

* set SymbolCell type correctly

* fix QueryAndExpressionsStep

* looks like the types were actually wrong instead :D

* undo this for now...

* remove spinner waits

* more robust tests

* rewrite errorboundary test to not explicitly count the number of renders

* make urlParam expect async

* increase timeout in waitFor

* revert ExplorePage test changes

* Update public/app/features/dashboard/containers/DashboardPage.test.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* Update public/app/features/dashboard/containers/PublicDashboardPage.test.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* Update public/app/features/dashboard/containers/PublicDashboardPage.test.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* Update public/app/features/dashboard/containers/PublicDashboardPage.test.tsx

Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>

* skip fakeTimer test, ignore table types for now + other review comments

* update package peerDeps

* small tweak to resourcepicker test

* update lockfile...

* increase timeout in sharepublicdashboard tests

* ensure ExplorePaneContainer passes correct queries to initializeExplore

* fix LokiContextUI test

* fix unit tests

* make importDashboard flow more consistent

* wait for dashboard name before continuing

* more test fixes

* readd dashboard name to variable e2e tests

* wait for switches to be enabled before clicking

* fix modal rendering

* don't use @testing-library/dom directly

* quick fix for rendering of panels in firefox

* make PromQueryField test more robust

* don't wait for chartData - in react 18 this can happen before the wait code even gets executed

---------

Co-authored-by: kay delaney <kay@grafana.com>
Co-authored-by: Alex Khomenko <Clarity-89@users.noreply.github.com>
@zerok zerok modified the milestones: 10.0.0, 10.0.0-preview May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet