ui: Upgrade React version and react-dependent libraries #43846

koorosh · 2020-01-09T14:06:54Z

Has to merged after: ui: Upgrade to Webpack@^4 and babel@^7 versions #43685
Depends on Upgrade react libraries to the latest version yarn-vendored#13

Current PR has affected a lot of files but most of changes are almost the same
and do not affect business logic.

With current changes, react is upgraded to latest version (16.12.0) and
following libraries are upgraded as well: react-redux, redux, redux-saga.
These upgrades of major versions has required to do some code adjustments
to properly migrate to newest versions.

redux. Provide valid type arguments for Dispatch type.
redux-saga. Changed import and usage of 'delay' function.
react-redux. connect function failed to correctly resolve returned type
for mapDispatchToProps argument. As result it is provided as a function
instead of object argument. This hack allows implicitly resolve types.
CachedDataReducer class is extended with one more optional type argument
to specify the list of allowed actionNamespace literals instead of string.
It was necessary to have strictly defined CachedDataReducer interface.
normalizeConnectedComponent function is used to do simple mapping from
react's ExoticComponent to valid react component. This resolves issue of
providing ConnectedComponents (which are ExoticComponent's, not regular react
components) to Route component which has strict type validation of component types.

cockroach-teamcity · 2020-01-09T14:07:09Z

This change is

koorosh · 2020-01-09T14:09:10Z

pkg/ui/ccl/src/views/shared/containers/licenseSwap/index.tsx

  const ossName = getComponentName(OSSComponent);
  const cclName = getComponentName(CCLComponent);

-  class LicenseSwap extends React.Component<TProps & OwnProps> {
+  class LicenseSwap extends React.Component<TProps & OwnProps & any> {


Following hack with &any type is required to make changes related to types only and do not rewrite component logic.

dhartunian

Thanks for this huge undertaking @koorosh! My only major concern is whether the restructuring the mapDispatchToProps is necessary. See my comments for the approach I looked at.

dhartunian · 2020-01-27T20:30:35Z

pkg/ui/src/redux/cachedDataReducer.ts

@@ -180,7 +180,7 @@ export class CachedDataReducer<TRequest, TResponseMessage> {
   *   state given the global state object
   */
  refresh = <S>(req?: TRequest, stateAccessor = (state: any, _req: TRequest) => state.cachedData[this.actionNamespace]) => {
-    return (dispatch: Dispatch<S>, getState: () => any) => {
+    return (dispatch: Dispatch<Action, TResponseMessage>, getState: () => S) => {


I noticed that in the rest of the PR that everywhere refreshNodes was used we needed to add the arrow function + bindActionCreators wrapping.

I tried adding a ThunkAction<any, S, any> type to the refresh function like below and it seemed to remove the need for the arrow function wrapping in the connect function, would you mind trying out?

refresh = <S>(req?: TRequest, stateAccessor = (state: any, _req: TRequest) => state.cachedData[this.actionNamespace]): ThunkAction<any, S, any> => { return (dispatch: Dispatch<Action, S>, getState: () => S) => {

Awesome, it makes code much cleaner! Thanks!

dhartunian · 2020-01-27T20:45:43Z

pkg/ui/src/views/cluster/containers/events/index.tsx

@@ -178,9 +176,9 @@ const eventBoxConnected = connect(
      eventsValid: eventsValidSelector(state),
    };
  },
-  {
+  () => ({


I think this is unnecessary and the one below.

Agree. Removed unnecessary function wrappers across all project

With current changes, react is upgraded to latest version (16.12.0) and following libraries are upgraded as well: react-redux, redux, redux-saga. These upgrades of major versions has required to do some code adjustments to properly migrate to newest versions. - redux. Provide valid type arguments for Dispatch type. - redux-saga. Changed import and usage of 'delay' function. - react-redux. `connect` function failed to correctly resolve returned type for `mapDispatchToProps` argument. As result it is provided as a function instead of object argument. This hack allows implicitly resolve types. - `CachedDataReducer` class is extended with one more optional type argument to specify the list of allowed `actionNamespace` literals instead of `string`. It was necessary to have strictly defined CachedDataReducer interface. - `normalizeConnectedComponent` function is used to do simple mapping from react's ExoticComponent to valid react component. This resolves issue of providing ConnectedComponents (which are ExoticComponent's, not regular react components) to Route component which has strict type validation of component types. Release note: None

Release note: None

- tests command is disabled for now because current state doesn't contain all fixes to run tests without failure. - Decommissioned Node History page was added so code has to be adjusted. Release note: None

Redux Actions for some components (ClusterOverview, NodesLogs, Statements page, and Node Maps) were not properly binded. As result component were not provided with data, and weren't rendered. To fix this, actions are wrapped with 'bindActionCreators' helper. Release note: None

- Issue: nfl/react-helmet#373 - Workaround: nfl/react-helmet#373 (comment) Release note: None

- get rid of unnecessary wrapping action creators with bindActionCreators function - define correct return type for cachedDataReducer.refresh action Release note: None

Release note: None

Remove useless function wrapper for action creator object Release note: None

Release note: None

dhartunian

👍 LGTM. Thanks for the updates!

dhartunian · 2020-01-29T18:44:50Z

bors r+

43846: ui: Upgrade React version and react-dependent libraries r=dhartunian a=Koorosh - Has to merged after: #43685 - Depends on cockroachdb/yarn-vendored#13 Current PR has affected a lot of files but most of changes are almost the same and do not affect business logic. With current changes, react is upgraded to latest version (16.12.0) and following libraries are upgraded as well: react-redux, redux, redux-saga. These upgrades of major versions has required to do some code adjustments to properly migrate to newest versions. - redux. Provide valid type arguments for Dispatch type. - redux-saga. Changed import and usage of 'delay' function. - react-redux. `connect` function failed to correctly resolve returned type for `mapDispatchToProps` argument. As result it is provided as a function instead of object argument. This hack allows implicitly resolve types. - `CachedDataReducer` class is extended with one more optional type argument to specify the list of allowed `actionNamespace` literals instead of `string`. It was necessary to have strictly defined CachedDataReducer interface. - `normalizeConnectedComponent` function is used to do simple mapping from react's ExoticComponent to valid react component. This resolves issue of providing ConnectedComponents (which are ExoticComponent's, not regular react components) to Route component which has strict type validation of component types. Co-authored-by: Andrii Vorobiov <[email protected]>

craig · 2020-01-29T19:16:39Z

Build succeeded

GitHub CI (Cockroach)

44078: ui: Tests on react-routing components r=dhartunian a=Koorosh Depends on PR: #43846 `yarn-vendor` PR has to be merged: cockroachdb/yarn-vendored#15 Current PR includes minimal changes to functionality (only those which are required to isolate or get access to individual components under tests): - `src/index.tsx` file has been refactored into two parts: rendering to DOM and independent `App` component; - <Helmet> component (`react-helmet` library) has been refactored to avoid known issue during tests execution (nfl/react-helmet#373) - old version of react-router and `connect` component in tandem do not work well because connected component aren't valid react components. As result validation for correct routing was done by re-exporting wrapped components (instead of connected). It allowed to simplify testing with `enzyme#find` function which requires valid react component as an argument; - Tests are grouped in suites per route to maintain related tests per route; - There is two kinds of tests which test correct rendering of component for specific test and another test which validates correct redirection for routes; - Tests can be refactored to be constructed from configuration to reduce repeated tests, but it will be actual after migrating to latest version of react-router lib. 44545: sqlbase: add table generator for edge case datums r=rohany a=rohany Part of work for #44322. This PR adds a testing utility to generate a table full of edge case datums to test a variety of things in the future. Release note: None Co-authored-by: Andrii Vorobiov <[email protected]> Co-authored-by: Rohan Yadav <[email protected]>

koorosh commented Jan 9, 2020

View reviewed changes

koorosh marked this pull request as ready for review January 9, 2020 15:28

koorosh requested a review from a team January 9, 2020 15:28

koorosh mentioned this pull request Jan 16, 2020

ui: Tests on react-routing components #44078

Merged

koorosh force-pushed the ui-react-version-upgrade branch from cd8f3e5 to 9198891 Compare January 23, 2020 11:54

dhartunian changed the base branch from master to staging January 27, 2020 16:38

dhartunian changed the base branch from staging to master January 27, 2020 16:39

dhartunian requested changes Jan 27, 2020

View reviewed changes

koorosh force-pushed the ui-react-version-upgrade branch from 2f6d900 to 478a7d9 Compare January 28, 2020 11:28

koorosh added 8 commits January 28, 2020 14:56

ui: Add license header for new file

763b2b6

Release note: None

ui: Resolve conflicts after rebasing against master

3ac9632

- tests command is disabled for now because current state doesn't contain all fixes to run tests without failure. - Decommissioned Node History page was added so code has to be adjusted. Release note: None

ui: Fix "Maximum call stack size" issue with react-helmet library

9c7e40f

- Issue: nfl/react-helmet#373 - Workaround: nfl/react-helmet#373 (comment) Release note: None

ui: Refactor mapping of actions to components

52cf853

- get rid of unnecessary wrapping action creators with bindActionCreators function - define correct return type for cachedDataReducer.refresh action Release note: None

ui: Align heading for network page with text

3f4a746

Release note: None

ui: Simplify mapping Action dispatchers to component properties

7f94c61

Remove useless function wrapper for action creator object Release note: None

koorosh force-pushed the ui-react-version-upgrade branch from c5d6e00 to 7f94c61 Compare January 28, 2020 12:56

ui: Update ref to yarn-vendor sub-module

0a960d3

Release note: None

koorosh requested a review from dhartunian January 28, 2020 16:45

dhartunian approved these changes Jan 29, 2020

View reviewed changes

craig bot merged commit 0a960d3 into cockroachdb:master Jan 29, 2020

dhartunian mentioned this pull request Jan 29, 2020

ui: Upgrade React version in the admin UI #43361

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Upgrade React version and react-dependent libraries #43846

ui: Upgrade React version and react-dependent libraries #43846

koorosh commented Jan 9, 2020 •

edited

Loading

cockroach-teamcity commented Jan 9, 2020

koorosh Jan 9, 2020

dhartunian left a comment

dhartunian Jan 27, 2020

koorosh Jan 28, 2020

dhartunian Jan 27, 2020

koorosh Jan 28, 2020

dhartunian left a comment

dhartunian commented Jan 29, 2020

craig bot commented Jan 29, 2020

ui: Upgrade React version and react-dependent libraries #43846

ui: Upgrade React version and react-dependent libraries #43846

Conversation

koorosh commented Jan 9, 2020 • edited Loading

cockroach-teamcity commented Jan 9, 2020

koorosh Jan 9, 2020

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

dhartunian Jan 27, 2020

Choose a reason for hiding this comment

koorosh Jan 28, 2020

Choose a reason for hiding this comment

dhartunian Jan 27, 2020

Choose a reason for hiding this comment

koorosh Jan 28, 2020

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

dhartunian commented Jan 29, 2020

craig bot commented Jan 29, 2020

Build succeeded

koorosh commented Jan 9, 2020 •

edited

Loading