From e66ef79a3df464b7625baad11683b6776cfd5c30 Mon Sep 17 00:00:00 2001 From: JF-Cozy Date: Tue, 20 Dec 2022 16:23:18 +0100 Subject: [PATCH] feat(harvest): Replace router dependency in component by context dep We want harvest to work without major changes with both a v3 router (Banks and Drive), v4 (use case "Home"), and with a v6 router (use case "MyPapers"). Initially we didn't want to have a backward compatibility with v3 and Banks because we thought it was autonomous and passed everything to harvest. But this is not the case. Moreover we initially thought of using a context to determine which version of the router to use, but this poses problems because there are several "entry points" to Harvest and we are not sure to have the context in all cases. The idea is that we remove the direct dependency on the router via `withRouter` in the components, and replace it with a global `withAdaptiveRouter` dependency that will use either version of the router. Some components were already using this semblance of an approach with the approach with `withMountPointHistory`. So we improved it by adding `location` to this HOC. --- .../src/components/AccountForm/index.jsx | 7 +- .../src/components/AccountModal.jsx | 15 ++-- .../src/components/EditAccountModal.jsx | 7 +- .../src/components/KonnectorAccounts.jsx | 16 ++-- .../src/components/KonnectorSuccess.jsx | 12 +-- .../src/components/MountPointContext.jsx | 33 +++----- .../src/components/MountPointContext.spec.jsx | 53 ------------ .../src/components/hoc/withRouter.jsx | 82 +++++++++++++++++++ .../src/components/hoc/withRouter.spec.jsx | 51 ++++++++++++ 9 files changed, 177 insertions(+), 99 deletions(-) delete mode 100644 packages/cozy-harvest-lib/src/components/MountPointContext.spec.jsx create mode 100644 packages/cozy-harvest-lib/src/components/hoc/withRouter.jsx create mode 100644 packages/cozy-harvest-lib/src/components/hoc/withRouter.spec.jsx diff --git a/packages/cozy-harvest-lib/src/components/AccountForm/index.jsx b/packages/cozy-harvest-lib/src/components/AccountForm/index.jsx index e02bc5e2bf..d997fa4dbe 100644 --- a/packages/cozy-harvest-lib/src/components/AccountForm/index.jsx +++ b/packages/cozy-harvest-lib/src/components/AccountForm/index.jsx @@ -1,5 +1,4 @@ import React, { PureComponent } from 'react' -import { withRouter } from 'react-router' import { Form } from 'react-final-form' import PropTypes from 'prop-types' import get from 'lodash/get' @@ -15,6 +14,7 @@ import { Media, Img, Bd } from 'cozy-ui/transpiled/react/Media' import Typography from 'cozy-ui/transpiled/react/Typography' import { Dialog } from 'cozy-ui/transpiled/react/CozyDialogs' +import withAdaptiveRouter from '../hoc/withRouter' import withLocales from '../hoc/withLocales' import AccountFields from './AccountFields' import ReadOnlyIdentifier from './ReadOnlyIdentifier' @@ -230,6 +230,7 @@ export class AccountForm extends PureComponent { showError, t, fieldOptions, + historyAction, flowState } = this.props const submitting = flowState.running @@ -343,7 +344,7 @@ export class AccountForm extends PureComponent { className="u-mt-2 u-mb-1-half" fullWidth label={t('accountForm.installFlagship.label')} - onClick={() => this.props.history.push('/')} + onClick={() => historyAction('/', 'push')} /> )} @@ -429,7 +430,7 @@ AccountForm.defaultProps = { } export default compose( - withRouter, + withAdaptiveRouter, withConnectionFlow(), withLocales, withKonnectorLocales diff --git a/packages/cozy-harvest-lib/src/components/AccountModal.jsx b/packages/cozy-harvest-lib/src/components/AccountModal.jsx index 7db5dfe45d..11c4d15710 100644 --- a/packages/cozy-harvest-lib/src/components/AccountModal.jsx +++ b/packages/cozy-harvest-lib/src/components/AccountModal.jsx @@ -14,7 +14,7 @@ import * as triggersModel from '../helpers/triggers' import KonnectorAccountTabs from './KonnectorConfiguration/KonnectorAccountTabs' import AccountSelectBox from './AccountSelectBox/AccountSelectBox' import KonnectorModalHeader from './KonnectorModalHeader' -import { withMountPointHistory } from './MountPointContext' +import { withMountPointProps } from './MountPointContext' import withLocales from './hoc/withLocales' import DialogContent from '@material-ui/core/DialogContent' import { @@ -53,6 +53,7 @@ export class AccountModal extends Component { fetching: true, error: null } + async componentDidMount() { await this.loadSelectedAccountId() } @@ -121,12 +122,12 @@ export class AccountModal extends Component { intentsApi, innerAccountModalOverrides } = this.props - const { trigger, account, fetching, error } = this.state + return ( <> - {showAccountSelection ? ( + {showAccountSelection && ( - ) : null} + )} - {error || fetching ? ( + {(error || fetching) && ( {error && ( )} - ) : null} + )} {!error && !fetching && ( @@ -123,4 +123,4 @@ KonnectorSuccess.propTypes = { export { SuccessImage, BanksLink, DriveLink } -export default withRouter(translate()(KonnectorSuccess)) +export default withAdaptiveRouter(translate()(KonnectorSuccess)) diff --git a/packages/cozy-harvest-lib/src/components/MountPointContext.jsx b/packages/cozy-harvest-lib/src/components/MountPointContext.jsx index 45cf04a5e4..cd5296c7ec 100644 --- a/packages/cozy-harvest-lib/src/components/MountPointContext.jsx +++ b/packages/cozy-harvest-lib/src/components/MountPointContext.jsx @@ -1,8 +1,9 @@ import React, { useContext } from 'react' import PropTypes from 'prop-types' -import { withRouter } from 'react-router' -const MountPointContext = React.createContext() +import withAdaptiveRouter from './hoc/withRouter' + +export const MountPointContext = React.createContext() export class RawMountPointProvider extends React.Component { constructor(props) { @@ -14,26 +15,17 @@ export class RawMountPointProvider extends React.Component { this.state = { baseRoute: props.baseRoute || '/', pushHistory: this.pushHistory, - replaceHistory: this.replaceHistory + replaceHistory: this.replaceHistory, + location: props.location } } - historyAction(route, method) { - const { history } = this.props - const { baseRoute } = this.state - const segments = '/'.concat( - baseRoute.split('/').concat(route.split('/')).filter(Boolean).join('/') - ) - - history[method](segments) - } - pushHistory(route) { - this.historyAction(route, 'push') + this.props.historyAction(route, 'push') } replaceHistory(route) { - this.historyAction(route, 'replace') + this.props.historyAction(route, 'replace') } render() { @@ -50,25 +42,26 @@ RawMountPointProvider.propTypes = { history: PropTypes.object } -const withMountPointHistory = BaseComponent => { +export const withMountPointProps = BaseComponent => { const Component = props => { - const { pushHistory, replaceHistory } = useContext(MountPointContext) + const { pushHistory, replaceHistory, location } = + useContext(MountPointContext) return ( ) } - Component.displayName = `withMountPointHistory(${ + Component.displayName = `withMountPointProps(${ BaseComponent.displayName || BaseComponent.name })` return Component } -const MountPointProvider = withRouter(RawMountPointProvider) -export { MountPointContext, MountPointProvider, withMountPointHistory } +export const MountPointProvider = withAdaptiveRouter(RawMountPointProvider) diff --git a/packages/cozy-harvest-lib/src/components/MountPointContext.spec.jsx b/packages/cozy-harvest-lib/src/components/MountPointContext.spec.jsx deleted file mode 100644 index 3584137f40..0000000000 --- a/packages/cozy-harvest-lib/src/components/MountPointContext.spec.jsx +++ /dev/null @@ -1,53 +0,0 @@ -import React from 'react' -import { shallow } from 'enzyme' -import { RawMountPointProvider } from 'components/MountPointContext' - -describe('MountPointProvider', () => { - const historyMock = { replace: jest.fn() } - - beforeEach(() => { - jest.resetAllMocks() - }) - - it('should push to the correct route', () => { - const component = shallow( - - ) - - component.instance().replaceHistory('/one') - expect(historyMock.replace).toHaveBeenCalledWith('/root/one') - - component.instance().replaceHistory('/one/two') - expect(historyMock.replace).toHaveBeenCalledWith('/root/one/two') - - component.instance().replaceHistory('no/slash') - expect(historyMock.replace).toHaveBeenCalledWith('/root/no/slash') - - component.instance().replaceHistory('too/many///slashes') - expect(historyMock.replace).toHaveBeenCalledWith('/root/too/many/slashes') - }) - - it('should handle a trailing slash in the base route', () => { - const component = shallow( - - ) - - component.instance().replaceHistory('/one') - expect(historyMock.replace).toHaveBeenCalledWith('/root/one') - - component.instance().replaceHistory('no/slash') - expect(historyMock.replace).toHaveBeenCalledWith('/root/no/slash') - }) - - it('should handle a base route with multiple segments', () => { - const component = shallow( - - ) - - component.instance().replaceHistory('/one') - expect(historyMock.replace).toHaveBeenCalledWith('/base/route/one') - - component.instance().replaceHistory('/one/two') - expect(historyMock.replace).toHaveBeenCalledWith('/base/route/one/two') - }) -}) diff --git a/packages/cozy-harvest-lib/src/components/hoc/withRouter.jsx b/packages/cozy-harvest-lib/src/components/hoc/withRouter.jsx new file mode 100644 index 0000000000..c948753537 --- /dev/null +++ b/packages/cozy-harvest-lib/src/components/hoc/withRouter.jsx @@ -0,0 +1,82 @@ +import React from 'react' +import flow from 'lodash/flow' + +import { getDisplayName } from './utils' + +// should be `react-router-dom` when `react-router` removed from peerDep +export const isRouterV6 = !require('react-router').withRouter + +export const historyAction = (props, historyOrNavigate) => (route, method) => { + const { baseRoute } = props + + if (isRouterV6) { + // in react-router v6 we don't want to use "/"" as first character of a route + // otherwise the path would be absolute and not relative + const path = route[0] === '/' ? route.substr(1) : route + return historyOrNavigate(path, { replace: method === 'replace' }) + } + + const segments = '/'.concat( + baseRoute.split('/').concat(route.split('/')).filter(Boolean).join('/') + ) + + return historyOrNavigate[method](segments) +} + +const withRouter = WrappedComponent => { + function Component(props) { + const { location: legacyLocation, history: legacyHistory } = props + + const adaptiveLocation = isRouterV6 + ? require('react-router-dom').useLocation() + : legacyLocation + + const historyOrNavigate = isRouterV6 + ? require('react-router-dom').useNavigate() + : require('react-router-dom').useHistory?.() || legacyHistory + + return ( + + ) + } + + Component.displayName = 'withRouter(' + getDisplayName(WrappedComponent) + ')' + Component.propTypes = { + ...WrappedComponent.propTypes + } + Component.WrappedComponent = WrappedComponent + + return Component +} + +/** + * HOC to wrap with a router version older than 6 methods if relevant + */ +const withAdaptiveRouter = WrappedComponent => { + // We use `react-router` here and not `react-router-dom` to ensure + // backward compatibility with Banks which uses `react-router` v3 for the moment. + // When Banks router migration is done to v6, we can replace with `react-router-dom`. + // There will then be a problem with the tests, we will have to correct them or add this to the jest configuration: + // '^react-router-dom$': '/node_modules/react-router/lib/index.js' + const withLegacyRouter = require('react-router').withRouter // withRouter is only available in `react-router < 6` + + const ComponentV4orV6 = withLegacyRouter + ? flow(withRouter, withLegacyRouter)(WrappedComponent) + : withRouter(WrappedComponent) + + function Component(props) { + return + } + + Component.displayName = `withAdaptiveRouter(${ + WrappedComponent.displayName || WrappedComponent.name + })` + + return Component +} + +export default withAdaptiveRouter diff --git a/packages/cozy-harvest-lib/src/components/hoc/withRouter.spec.jsx b/packages/cozy-harvest-lib/src/components/hoc/withRouter.spec.jsx new file mode 100644 index 0000000000..d65b6e0442 --- /dev/null +++ b/packages/cozy-harvest-lib/src/components/hoc/withRouter.spec.jsx @@ -0,0 +1,51 @@ +import { historyAction } from './withRouter' + +const historyMock = { replace: jest.fn(), push: jest.fn() } + +const setup = ({ baseRoute, route, method = 'replace' } = {}) => { + historyAction({ baseRoute }, historyMock)(route, method) +} + +describe('historyAction', () => { + afterEach(() => { + jest.resetAllMocks() + }) + + it('should push the correct route', () => { + setup({ baseRoute: '/root', route: '/one' }) + expect(historyMock.replace).toHaveBeenCalledWith('/root/one') + historyMock.replace.mockReset() + + setup({ baseRoute: '/root', route: '/one/two' }) + expect(historyMock.replace).toHaveBeenCalledWith('/root/one/two') + historyMock.replace.mockReset() + + setup({ baseRoute: '/root', route: 'no/slash' }) + expect(historyMock.replace).toHaveBeenCalledWith('/root/no/slash') + historyMock.replace.mockReset() + + setup({ baseRoute: '/root', route: 'too/many///slashes' }) + expect(historyMock.replace).toHaveBeenCalledWith('/root/too/many/slashes') + historyMock.replace.mockReset() + }) + + it('should handle a trailing slash in the base route', () => { + setup({ baseRoute: '/root/', route: '/one' }) + expect(historyMock.replace).toHaveBeenCalledWith('/root/one') + historyMock.replace.mockReset() + + setup({ baseRoute: '/root/', route: 'no/slash' }) + expect(historyMock.replace).toHaveBeenCalledWith('/root/no/slash') + historyMock.replace.mockReset() + }) + + it('should handle a base route with multiple segments', () => { + setup({ baseRoute: '/base/route/', route: '/one' }) + expect(historyMock.replace).toHaveBeenCalledWith('/base/route/one') + historyMock.replace.mockReset() + + setup({ baseRoute: '/base/route/', route: '/one/two' }) + expect(historyMock.replace).toHaveBeenCalledWith('/base/route/one/two') + historyMock.replace.mockReset() + }) +})