From 13c0268d6bc8782a96a75167b20c3a98291327ee Mon Sep 17 00:00:00 2001 From: Ashley Harrison Date: Tue, 31 Oct 2023 16:24:34 +0000 Subject: [PATCH] Chore: Upgrade `react-redux` (#77296) * try upgrading react-redux * memoize getNavModel selector, don't return a new object in command palette selector * use createSelector to memoize some alert manager selectors correctly * memoize selectors correctly in appNotifications * memoize some datasource selectors * use fake timers to avoid the debounce causing flakiness * remove duplicate import * just use memoize-one --- .github/renovate.json5 | 1 - package.json | 3 +- public/app/core/reducers/appNotification.ts | 12 +-- public/app/core/selectors/navModel.ts | 34 ++++---- .../rule-editor/DashboardPicker.test.tsx | 13 ++- .../unified/hooks/useExternalAmSelector.ts | 11 +-- .../hooks/useUnifiedAlertingSelector.ts | 12 ++- .../commandPalette/actions/useActions.ts | 7 +- .../features/datasources/state/selectors.ts | 10 ++- yarn.lock | 79 +++++++++++++------ 10 files changed, 116 insertions(+), 66 deletions(-) diff --git a/.github/renovate.json5 b/.github/renovate.json5 index 5ee322b31a32b..e8f9da4aba23a 100644 --- a/.github/renovate.json5 +++ b/.github/renovate.json5 @@ -8,7 +8,6 @@ "react-router-dom", // we should bump this together with history (see https://github.com/grafana/grafana/issues/76744) "monaco-editor", // due to us exposing this via @grafana/ui/CodeEditor's props bumping can break plugins "react-hook-form", // due to us exposing these hooks via @grafana/ui form components bumping can break plugins - "react-redux", // react-beautiful-dnd depends on react-redux 7.x, we need to update that one first ], "includePaths": ["package.json", "packages/**", "public/app/plugins/**"], "ignorePaths": ["emails/**", "plugins-bundled/**", "**/mocks/**", "packages/grafana-e2e/**"], diff --git a/package.json b/package.json index ab33dfbdc00a5..14ed107f5e2f9 100644 --- a/package.json +++ b/package.json @@ -135,7 +135,6 @@ "@types/react-dom": "18.2.7", "@types/react-grid-layout": "1.3.2", "@types/react-highlight-words": "0.16.4", - "@types/react-redux": "7.1.25", "@types/react-router-dom": "5.3.3", "@types/react-table": "7.7.14", "@types/react-test-renderer": "18.0.0", @@ -382,7 +381,7 @@ "react-moveable": "0.46.1", "react-popper": "2.3.0", "react-popper-tooltip": "4.4.2", - "react-redux": "7.2.6", + "react-redux": "8.1.3", "react-resizable": "3.0.5", "react-responsive-carousel": "^3.2.23", "react-router-dom": "5.3.3", diff --git a/public/app/core/reducers/appNotification.ts b/public/app/core/reducers/appNotification.ts index 5f88fd41e9930..d9f563d364e31 100644 --- a/public/app/core/reducers/appNotification.ts +++ b/public/app/core/reducers/appNotification.ts @@ -1,4 +1,4 @@ -import { createSlice, PayloadAction } from '@reduxjs/toolkit'; +import { createSelector, createSlice, PayloadAction } from '@reduxjs/toolkit'; import { AppNotification, AppNotificationSeverity, AppNotificationsState } from 'app/types/'; @@ -60,10 +60,12 @@ export const appNotificationsReducer = appNotificationsSlice.reducer; // Selectors export const selectLastReadTimestamp = (state: AppNotificationsState) => state.lastRead; -export const selectAll = (state: AppNotificationsState) => - Object.values(state.byId).sort((a, b) => b.timestamp - a.timestamp); -export const selectWarningsAndErrors = (state: AppNotificationsState) => selectAll(state).filter(isAtLeastWarning); -export const selectVisible = (state: AppNotificationsState) => Object.values(state.byId).filter((n) => n.showing); +export const selectById = (state: AppNotificationsState) => state.byId; +export const selectAll = createSelector(selectById, (byId) => + Object.values(byId).sort((a, b) => b.timestamp - a.timestamp) +); +export const selectWarningsAndErrors = createSelector(selectAll, (all) => all.filter(isAtLeastWarning)); +export const selectVisible = createSelector(selectById, (byId) => Object.values(byId).filter((n) => n.showing)); // Helper functions diff --git a/public/app/core/selectors/navModel.ts b/public/app/core/selectors/navModel.ts index c7c15a4fc74b9..f4f04de999466 100644 --- a/public/app/core/selectors/navModel.ts +++ b/public/app/core/selectors/navModel.ts @@ -1,3 +1,5 @@ +import memoizeOne from 'memoize-one'; + import { NavModel, NavModelItem, NavIndex } from '@grafana/data'; import { newBrowseDashboardsEnabled } from 'app/features/browse-dashboards/featureFlag'; import { FOLDER_ID } from 'app/features/folders/state/navModel'; @@ -19,24 +21,26 @@ const getNotFoundModel = (): NavModel => { }; }; -export const getNavModel = (navIndex: NavIndex, id: string, fallback?: NavModel, onlyChild = false): NavModel => { - if (navIndex[id]) { - const node = navIndex[id]; - const main = onlyChild ? node : getRootSectionForNode(node); - const mainWithActive = enrichNodeWithActiveState(main, id); +export const getNavModel = memoizeOne( + (navIndex: NavIndex, id: string, fallback?: NavModel, onlyChild = false): NavModel => { + if (navIndex[id]) { + const node = navIndex[id]; + const main = onlyChild ? node : getRootSectionForNode(node); + const mainWithActive = enrichNodeWithActiveState(main, id); - return { - node: node, - main: mainWithActive, - }; - } + return { + node: node, + main: mainWithActive, + }; + } - if (fallback) { - return fallback; - } + if (fallback) { + return fallback; + } - return getNotFoundModel(); -}; + return getNotFoundModel(); + } +); export function getRootSectionForNode(node: NavModelItem): NavModelItem { // Don't recurse fully up the folder tree when nested folders is enabled diff --git a/public/app/features/alerting/unified/components/rule-editor/DashboardPicker.test.tsx b/public/app/features/alerting/unified/components/rule-editor/DashboardPicker.test.tsx index 54ae7d967f2fe..4257ac8c82de7 100644 --- a/public/app/features/alerting/unified/components/rule-editor/DashboardPicker.test.tsx +++ b/public/app/features/alerting/unified/components/rule-editor/DashboardPicker.test.tsx @@ -1,4 +1,4 @@ -import { render } from '@testing-library/react'; +import { act, render } from '@testing-library/react'; import { noop } from 'lodash'; import React from 'react'; import { AutoSizerProps } from 'react-virtualized-auto-sizer'; @@ -38,10 +38,21 @@ const ui = { }; describe('DashboardPicker', () => { + beforeEach(() => { + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); + }); + it('Renders panels without ids', async () => { render(, { wrapper: TestProvider, }); + act(() => { + jest.advanceTimersByTime(500); + }); expect(await ui.dashboardButton(/Dashboard 1/).find()).toBeInTheDocument(); expect(await ui.dashboardButton(/Dashboard 2/).find()).toBeInTheDocument(); diff --git a/public/app/features/alerting/unified/hooks/useExternalAmSelector.ts b/public/app/features/alerting/unified/hooks/useExternalAmSelector.ts index 65f20eb0384fb..8ffe85bb8cb22 100644 --- a/public/app/features/alerting/unified/hooks/useExternalAmSelector.ts +++ b/public/app/features/alerting/unified/hooks/useExternalAmSelector.ts @@ -1,8 +1,9 @@ import { countBy, keyBy } from 'lodash'; +import { createSelector } from 'reselect'; import { DataSourceInstanceSettings, DataSourceJsonData, DataSourceSettings } from '@grafana/data'; import { AlertManagerDataSourceJsonData } from 'app/plugins/datasource/alertmanager/types'; -import { useSelector } from 'app/types'; +import { StoreState, useSelector } from 'app/types'; import { alertmanagerApi } from '../api/alertmanagerApi'; import { getAlertManagerDataSources } from '../utils/datasource'; @@ -20,10 +21,10 @@ export function useExternalDataSourceAlertmanagers(): ExternalDataSourceAM[] { const externalDsAlertManagers = getAlertManagerDataSources().filter((ds) => ds.jsonData.handleGrafanaManagedAlerts); - const alertmanagerDatasources = useSelector((state) => - keyBy( - state.dataSources.dataSources.filter((ds) => ds.type === 'alertmanager'), - (ds) => ds.uid + const alertmanagerDatasources = useSelector( + createSelector( + (state: StoreState) => state.dataSources.dataSources.filter((ds) => ds.type === 'alertmanager'), + (datasources) => keyBy(datasources, (ds) => ds.uid) ) ); diff --git a/public/app/features/alerting/unified/hooks/useUnifiedAlertingSelector.ts b/public/app/features/alerting/unified/hooks/useUnifiedAlertingSelector.ts index 34d6e3aa08615..7b50599934c45 100644 --- a/public/app/features/alerting/unified/hooks/useUnifiedAlertingSelector.ts +++ b/public/app/features/alerting/unified/hooks/useUnifiedAlertingSelector.ts @@ -1,4 +1,6 @@ -import { useSelector } from 'app/types'; +import { createSelector } from 'reselect'; + +import { StoreState, useSelector } from 'app/types'; import { UnifiedAlertingState } from '../state/reducers'; @@ -6,5 +8,11 @@ export function useUnifiedAlertingSelector( selector: (state: UnifiedAlertingState) => TSelected, equalityFn?: (left: TSelected, right: TSelected) => boolean ): TSelected { - return useSelector((state) => selector(state.unifiedAlerting), equalityFn); + return useSelector( + createSelector( + (state: StoreState) => state.unifiedAlerting, + (unifiedAlerting) => selector(unifiedAlerting) + ), + equalityFn + ); } diff --git a/public/app/features/commandPalette/actions/useActions.ts b/public/app/features/commandPalette/actions/useActions.ts index a3895f694cade..19e83721171cb 100644 --- a/public/app/features/commandPalette/actions/useActions.ts +++ b/public/app/features/commandPalette/actions/useActions.ts @@ -11,11 +11,8 @@ export default function useActions(searchQuery: string) { const [navTreeActions, setNavTreeActions] = useState([]); const [recentDashboardActions, setRecentDashboardActions] = useState([]); - const { navBarTree } = useSelector((state) => { - return { - navBarTree: state.navBarTree, - }; - }); + const navBarTree = useSelector((state) => state.navBarTree); + // Load standard static actions useEffect(() => { const staticActionsResp = getStaticActions(navBarTree); diff --git a/public/app/features/datasources/state/selectors.ts b/public/app/features/datasources/state/selectors.ts index 3585de24db051..53be4a748ddb3 100644 --- a/public/app/features/datasources/state/selectors.ts +++ b/public/app/features/datasources/state/selectors.ts @@ -1,7 +1,9 @@ +import memoizeOne from 'memoize-one'; + import { DataSourcePluginMeta, DataSourceSettings, UrlQueryValue } from '@grafana/data'; import { DataSourcesState } from 'app/types/datasources'; -export const getDataSources = (state: DataSourcesState) => { +export const getDataSources = memoizeOne((state: DataSourcesState) => { const regex = new RegExp(state.searchQuery, 'i'); const filteredDataSources = state.dataSources.filter((dataSource: DataSourceSettings) => { @@ -11,15 +13,15 @@ export const getDataSources = (state: DataSourcesState) => { return filteredDataSources.sort((a, b) => state.isSortAscending ? a.name.localeCompare(b.name) : b.name.localeCompare(a.name) ); -}; +}); -export const getFilteredDataSourcePlugins = (state: DataSourcesState) => { +export const getFilteredDataSourcePlugins = memoizeOne((state: DataSourcesState) => { const regex = new RegExp(state.dataSourceTypeSearchQuery, 'i'); return state.plugins.filter((type: DataSourcePluginMeta) => { return regex.test(type.name); }); -}; +}); export const getDataSource = (state: DataSourcesState, dataSourceId: UrlQueryValue): DataSourceSettings => { if (state.dataSource.uid === dataSourceId) { diff --git a/yarn.lock b/yarn.lock index 2c5ccc663bd71..7551e3fc6546e 100644 --- a/yarn.lock +++ b/yarn.lock @@ -6118,10 +6118,10 @@ __metadata: languageName: node linkType: hard -"@remix-run/router@npm:1.10.0": - version: 1.10.0 - resolution: "@remix-run/router@npm:1.10.0" - checksum: 61f5e6374f2d0edd177a61c8a44715290d9197a61362505fb0784f948a39e8353d9604ce3747151f837fc7b950e1fbd71bc797b161643325c545037ffbedd134 +"@remix-run/router@npm:1.5.0": + version: 1.5.0 + resolution: "@remix-run/router@npm:1.5.0" + checksum: 3da27d64519df94919020f4b42aaa016f26891331be98468cbc807bc4d2cfb401d7e47d4f88a4a3d777fc3af23d162c668357a8e5d2c5947acdbca7b691bc325 languageName: node linkType: hard @@ -8501,6 +8501,16 @@ __metadata: languageName: node linkType: hard +"@types/hoist-non-react-statics@npm:^3.3.1": + version: 3.3.4 + resolution: "@types/hoist-non-react-statics@npm:3.3.4" + dependencies: + "@types/react": "npm:*" + hoist-non-react-statics: "npm:^3.3.0" + checksum: dee430941a9ea16b7f665ecafa9b134066a49d13ae497fc051cf5d41b3aead394ab1a8179c3c98c9a3584f80aed16fab82dd7979c7dcddfbb5f74a132575d362 + languageName: node + linkType: hard + "@types/html-minifier-terser@npm:^6.0.0": version: 6.0.0 resolution: "@types/html-minifier-terser@npm:6.0.0" @@ -8916,7 +8926,7 @@ __metadata: languageName: node linkType: hard -"@types/react-redux@npm:7.1.25, @types/react-redux@npm:^7.1.20": +"@types/react-redux@npm:^7.1.20": version: 7.1.25 resolution: "@types/react-redux@npm:7.1.25" dependencies: @@ -9236,6 +9246,13 @@ __metadata: languageName: node linkType: hard +"@types/use-sync-external-store@npm:^0.0.3": + version: 0.0.3 + resolution: "@types/use-sync-external-store@npm:0.0.3" + checksum: 161ddb8eec5dbe7279ac971531217e9af6b99f7783213566d2b502e2e2378ea19cf5e5ea4595039d730aa79d3d35c6567d48599f69773a02ffcff1776ec2a44e + languageName: node + linkType: hard + "@types/uuid@npm:9.0.2": version: 9.0.2 resolution: "@types/uuid@npm:9.0.2" @@ -17189,7 +17206,6 @@ __metadata: "@types/react-dom": "npm:18.2.7" "@types/react-grid-layout": "npm:1.3.2" "@types/react-highlight-words": "npm:0.16.4" - "@types/react-redux": "npm:7.1.25" "@types/react-resizable": "npm:3.0.4" "@types/react-router-dom": "npm:5.3.3" "@types/react-table": "npm:7.7.14" @@ -17358,7 +17374,7 @@ __metadata: react-moveable: "npm:0.46.1" react-popper: "npm:2.3.0" react-popper-tooltip: "npm:4.4.2" - react-redux: "npm:7.2.6" + react-redux: "npm:8.1.3" react-refresh: "npm:0.14.0" react-resizable: "npm:3.0.5" react-responsive-carousel: "npm:^3.2.23" @@ -24984,24 +25000,35 @@ __metadata: languageName: node linkType: hard -"react-redux@npm:7.2.6": - version: 7.2.6 - resolution: "react-redux@npm:7.2.6" +"react-redux@npm:8.1.3": + version: 8.1.3 + resolution: "react-redux@npm:8.1.3" dependencies: - "@babel/runtime": "npm:^7.15.4" - "@types/react-redux": "npm:^7.1.20" + "@babel/runtime": "npm:^7.12.1" + "@types/hoist-non-react-statics": "npm:^3.3.1" + "@types/use-sync-external-store": "npm:^0.0.3" hoist-non-react-statics: "npm:^3.3.2" - loose-envify: "npm:^1.4.0" - prop-types: "npm:^15.7.2" - react-is: "npm:^17.0.2" + react-is: "npm:^18.0.0" + use-sync-external-store: "npm:^1.0.0" peerDependencies: - react: ^16.8.3 || ^17 + "@types/react": ^16.8 || ^17.0 || ^18.0 + "@types/react-dom": ^16.8 || ^17.0 || ^18.0 + react: ^16.8 || ^17.0 || ^18.0 + react-dom: ^16.8 || ^17.0 || ^18.0 + react-native: ">=0.59" + redux: ^4 || ^5.0.0-beta.0 peerDependenciesMeta: + "@types/react": + optional: true + "@types/react-dom": + optional: true react-dom: optional: true react-native: optional: true - checksum: 62fed236e8ce022c7fd45c9fd5e26893d9154b5a53bb8aea2989fb5e82e938fe23414329510a3efe6a75c755da446f35bad78db73c5e839f87839a79b091e6c1 + redux: + optional: true + checksum: c4c7586cff3abeb784e73598d330f5301116a4e9942fd36895f2bccd8990001709c6c3ea1817edb75ee477470d6c67c9113e05a7f86b2b68a3950c9c29fe20cb languageName: node linkType: hard @@ -25111,16 +25138,16 @@ __metadata: linkType: hard "react-router-dom-v5-compat@npm:^6.10.0": - version: 6.17.0 - resolution: "react-router-dom-v5-compat@npm:6.17.0" + version: 6.10.0 + resolution: "react-router-dom-v5-compat@npm:6.10.0" dependencies: history: "npm:^5.3.0" - react-router: "npm:6.17.0" + react-router: "npm:6.10.0" peerDependencies: react: ">=16.8" react-dom: ">=16.8" react-router-dom: 4 || 5 - checksum: 77062bcdf2f1a5789d146e42d29e24f45074a49e6a1467ea7e5a57d2e50a961ebe1d9a219138bf3feef1e577d9635dd8d15d0e0f5851e574efe44a479441e261 + checksum: b86edb22640e25687a843fd46c22b58e147316ee957653e684d9f46a7e71da113a761a2f74c60d42f5d4222e497f825e541a1f4d8872b26edb4b623fb7c87928 languageName: node linkType: hard @@ -25161,14 +25188,14 @@ __metadata: languageName: node linkType: hard -"react-router@npm:6.17.0": - version: 6.17.0 - resolution: "react-router@npm:6.17.0" +"react-router@npm:6.10.0": + version: 6.10.0 + resolution: "react-router@npm:6.10.0" dependencies: - "@remix-run/router": "npm:1.10.0" + "@remix-run/router": "npm:1.5.0" peerDependencies: react: ">=16.8" - checksum: 5c589c67b53cc1a210bd08285392e951a2c3d51a2502806f68d9ce604307944239b0a3b766d8390b484d707ace3068af858e999a1c242662b917ddcd4ab3c453 + checksum: 3c78db213d2c67c7ae06125b296889ebf3407963268ef23319312b4d7bf455ecfaa59164be73d6b4e19fb2ef6c2771d7dfe764d5a91cbdbb7c8e84c95aca99cc languageName: node linkType: hard