Skip to content

Commit 9cbaa1b

Browse files
JohnC-80JohnC-80
authored
STCOR-952 break QueryStateUpdates out to their own class-based component. (#1604)
Changes from [App reordering](https://folio-org.atlassian.net/browse/STCOR-936) - specifically a functional refactor of `MainNav` unearthed a handful of errors at the app level, causing behaviors with logic that updates query parameters/location. This PR takes the previous code from the class-based `MainNav` and places it in its own component: `QueryStateUpdater` - naming aside, this removes the potentially offending logic from `MainNav`, leaving it to the task of simply rendering the navigation/menus of the UI. The useEffect implementations of the functional `MainNav` were written to mirror the `componentDidMount` and `componentDidUpdate` lifecycle methods of the class-based version. When the `location` was added to the `cDM` dependency array in [This PR](#1600), this meant that the logic would no longer execute stricly on mount/dismount - but anytime that the `location` updated... this would cause page refreshes, as the logic would find a black `prevQueryState` every time and always update the location, causing a refresh. `componentDidUpdate` carried around its `prevProps`, unlike functional react, where a state might be needed to accomplish such a comparison - but this doesn't seem like an ideal solution for the functional version, so here we are. This is better because separately concerned logic is separated... worse because the separated logic still has all of its tentacles into the App layer and seems like a great example of a component that does too much - why wouldn't/couldn't updating queryState/location actually just live at the application level somehow? A question for future FOLIO...
1 parent 97884c1 commit 9cbaa1b

6 files changed

+194
-54
lines changed

‎src/RootWithIntl.js

+2
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ import {
3636
AppCtxMenuProvider,
3737
SessionEventContainer,
3838
AppOrderProvider,
39+
QueryStateUpdater
3940
} from './components';
4041
import StaleBundleWarning from './components/StaleBundleWarning';
4142
import { StripesContext } from './StripesContext';
@@ -64,6 +65,7 @@ const RootWithIntl = ({ stripes, token = '', isAuthenticated = false, disableAut
6465
<Router history={history}>
6566
{ isAuthenticated || token || disableAuth ?
6667
<>
68+
<QueryStateUpdater stripes={connectedStripes} queryClient={queryClient} />
6769
<MainContainer>
6870
<AppCtxMenuProvider>
6971
<AppOrderProvider>

‎src/RootWithIntl.test.js

+1
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ jest.mock('./components/ModuleContainer', () => ({ children }) => children);
1414
jest.mock('./components/MainContainer', () => ({ children }) => children);
1515
jest.mock('./components/StaleBundleWarning', () => () => '<StaleBundleWarning>');
1616
jest.mock('./components/SessionEventContainer', () => () => '<SessionEventContainer>');
17+
jest.mock('./components/MainNav/QueryStateUpdater', () => () => null);
1718

1819
const defaultHistory = createMemoryHistory();
1920

‎src/components/MainNav/MainNav.js

+1-54
Original file line numberDiff line numberDiff line change
@@ -1,19 +1,8 @@
11
import React, { useEffect, useRef, useState } from 'react';
2-
import { isEqual } from 'lodash';
32
import { useIntl } from 'react-intl';
4-
import { useLocation, useHistory } from 'react-router-dom';
5-
import { useQueryClient } from 'react-query';
63

74
import { branding } from 'stripes-config';
85
import { Icon } from '@folio/stripes-components';
9-
import {
10-
updateQueryResource,
11-
getLocationQuery,
12-
updateLocation,
13-
getCurrentModule,
14-
isQueryResourceModule,
15-
getQueryResourceState,
16-
} from '../../locationService';
176

187
import css from './MainNav.css';
198
import NavButton from './NavButton';
@@ -22,65 +11,23 @@ import { CurrentAppGroup } from './CurrentApp';
2211
import ProfileDropdown from './ProfileDropdown';
2312
import AppList from './AppList';
2413
import { SkipLink } from './components';
25-
2614
import { useAppOrderContext } from './AppOrderProvider';
27-
2815
import { useStripes } from '../../StripesContext';
29-
import { useModules } from '../../ModulesContext';
3016

3117
const MainNav = () => {
3218
const {
3319
apps,
3420
} = useAppOrderContext();
35-
const queryClient = useQueryClient();
3621
const stripes = useStripes();
37-
const location = useLocation();
38-
const modules = useModules();
39-
const history = useHistory();
4022
const intl = useIntl();
4123

42-
const [curModule, setCurModule] = useState(getCurrentModule(modules, location));
4324
const [selectedApp, setSelectedApp] = useState(apps.find(entry => entry.active));
4425
const helpUrl = useRef(stripes.config.helpUrl ?? 'https://docs.folio.org').current;
4526

46-
useEffect(() => {
47-
let curQuery = getLocationQuery(location);
48-
const prevQueryState = {};
49-
50-
const { store } = stripes;
51-
const _unsubscribe = store.subscribe(() => {
52-
const module = curModule;
53-
54-
if (module && isQueryResourceModule(module, location)) {
55-
const { moduleName } = module;
56-
const queryState = getQueryResourceState(module, stripes.store);
57-
58-
// only update location if query state has changed
59-
if (!isEqual(queryState, prevQueryState[moduleName])) {
60-
curQuery = updateLocation(module, curQuery, stripes.store, history, location);
61-
prevQueryState[moduleName] = queryState;
62-
}
63-
}
64-
});
65-
66-
// remove QueryProvider cache to be 100% sure we're starting from a clean slate.
67-
queryClient.removeQueries();
68-
69-
return () => {
70-
_unsubscribe();
71-
};
72-
}, [location]); // eslint-disable-line
73-
74-
// if the location changes, we need to update the current module/query resource.
7527
// This logic changes the visible current app at the starting side of the Main Navigation.
7628
useEffect(() => {
7729
setSelectedApp(apps.find(entry => entry.active));
78-
const nextCurModule = getCurrentModule(modules, location);
79-
if (nextCurModule) {
80-
setCurModule(getCurrentModule(modules, location));
81-
updateQueryResource(location, nextCurModule, stripes.store);
82-
}
83-
}, [modules, location, stripes.store, apps]);
30+
}, [apps]);
8431

8532
return (
8633
<header className={css.navRoot} style={branding?.style?.mainNav ?? {}}>

‎src/components/MainNav/QueryStateUpdater.js

+99
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
import React from 'react';
2+
import PropTypes from 'prop-types';
3+
import { compose } from 'redux';
4+
import { injectIntl } from 'react-intl';
5+
import { withRouter } from 'react-router';
6+
import { isEqual } from 'lodash';
7+
8+
import { withModules } from '../Modules';
9+
10+
import {
11+
updateQueryResource,
12+
getLocationQuery,
13+
updateLocation,
14+
getCurrentModule,
15+
isQueryResourceModule,
16+
getQueryResourceState,
17+
} from '../../locationService';
18+
19+
/** QueryStateUpdater
20+
* onMount of stripes, sync the query state to the location.
21+
* Also if the location itself changes, sync the query resource to the current location.
22+
*/
23+
24+
class QueryStateUpdater extends React.Component {
25+
static propTypes = {
26+
history: PropTypes.shape({
27+
listen: PropTypes.func.isRequired,
28+
replace: PropTypes.func.isRequired,
29+
push: PropTypes.func.isRequired,
30+
}).isRequired,
31+
location: PropTypes.shape({
32+
pathname: PropTypes.string,
33+
}).isRequired,
34+
modules: PropTypes.shape({
35+
app: PropTypes.arrayOf(PropTypes.object),
36+
}),
37+
queryClient: PropTypes.shape({
38+
removeQueries: PropTypes.func.isRequired,
39+
}).isRequired,
40+
stripes: PropTypes.shape({
41+
store: PropTypes.shape({
42+
subscribe: PropTypes.func.isRequired,
43+
}),
44+
}),
45+
children: PropTypes.oneOfType([
46+
PropTypes.arrayOf(PropTypes.node),
47+
PropTypes.node,
48+
]),
49+
}
50+
51+
constructor(props) {
52+
super(props);
53+
this.store = props.stripes.store;
54+
}
55+
56+
componentDidMount() {
57+
let curQuery = getLocationQuery(this.props.location);
58+
const prevQueryState = {};
59+
60+
this._unsubscribe = this.store.subscribe(() => {
61+
const { history, location } = this.props;
62+
const module = this.curModule;
63+
64+
if (module && isQueryResourceModule(module, location)) {
65+
const { moduleName } = module;
66+
const queryState = getQueryResourceState(module, this.store);
67+
68+
// only update location if query state has changed
69+
if (!isEqual(queryState, prevQueryState[moduleName])) {
70+
curQuery = updateLocation(module, curQuery, this.store, history, location);
71+
prevQueryState[moduleName] = queryState;
72+
}
73+
}
74+
});
75+
76+
// remove QueryProvider cache to be 100% sure we're starting from a clean slate.
77+
this.props.queryClient.removeQueries();
78+
}
79+
80+
componentDidUpdate(prevProps) {
81+
const { modules, location } = this.props;
82+
this.curModule = getCurrentModule(modules, location);
83+
if (this.curModule && !isEqual(location, prevProps.location)) {
84+
updateQueryResource(location, this.curModule, this.store);
85+
}
86+
}
87+
88+
componentWillUnmount() {
89+
this._unsubscribe();
90+
}
91+
92+
render = () => this.props.children;
93+
}
94+
95+
export default compose(
96+
injectIntl,
97+
withRouter,
98+
withModules,
99+
)(QueryStateUpdater);

‎src/components/MainNav/QueryStateUpdater.test.js

+90
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,90 @@
1+
2+
import { render } from '@folio/jest-config-stripes/testing-library/react';
3+
import { QueryClientProvider, QueryClient } from 'react-query';
4+
import { MemoryRouter as Router, Route } from 'react-router-dom';
5+
import QueryStateUpdater from './QueryStateUpdater';
6+
7+
const mockUpdateQueryResource = jest.fn();
8+
jest.mock('../../locationService', () => ({
9+
...jest.requireActual('../../locationService'),
10+
updateLocation: jest.fn(),
11+
updateQueryResource: jest.fn(() => mockUpdateQueryResource()),
12+
getCurrentModule: jest.fn(() => true)
13+
}));
14+
15+
const mockUnsubscribe = jest.fn();
16+
const stripes = {
17+
store: {
18+
subscribe: jest.fn(() => mockUnsubscribe)
19+
}
20+
};
21+
22+
describe('QueryStateUpdater', () => {
23+
let qc;
24+
let wrapper;
25+
beforeAll(() => {
26+
qc = new QueryClient();
27+
wrapper = (testLocation = { pathname: '/initial' }) => ({ children }) => {
28+
qc = new QueryClient();
29+
return (
30+
<Router initialEntries={['/initial']}>
31+
<QueryClientProvider client={qc}>
32+
<Route
33+
path="*"
34+
location={testLocation}
35+
>
36+
{children}
37+
</Route>
38+
</QueryClientProvider>
39+
</Router>
40+
);
41+
};
42+
});
43+
it('renders', () => {
44+
expect(() => render(
45+
<QueryStateUpdater queryClient={qc} stripes={stripes} />,
46+
{ wrapper: wrapper() }
47+
)).not.toThrow();
48+
});
49+
50+
it('updatesQuery on mount', () => {
51+
let testVal = {
52+
hash: '',
53+
key: 'f727ww',
54+
pathname: '/initial',
55+
search: '',
56+
state: undefined
57+
};
58+
const { rerender } = render(
59+
<QueryStateUpdater
60+
stripes={stripes}
61+
queryClient={qc}
62+
/>,
63+
{ wrapper: wrapper(testVal) }
64+
);
65+
testVal = {
66+
hash: '',
67+
key: 'f727w2',
68+
pathname: '/updated',
69+
search: '',
70+
state: undefined
71+
};
72+
rerender(
73+
<QueryStateUpdater
74+
stripes={stripes}
75+
queryClient={qc}
76+
location={testVal}
77+
/>,
78+
{ wrapper: wrapper(testVal) }
79+
);
80+
81+
expect(() => rerender(
82+
<QueryStateUpdater
83+
stripes={stripes}
84+
queryClient={qc}
85+
location={testVal}
86+
/>,
87+
{ wrapper: wrapper(testVal) }
88+
)).not.toThrow();
89+
});
90+
});

‎src/components/index.js

+1
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@ export { default as Login } from './Login';
88
export { default as Logout } from './Logout';
99
export { default as MainContainer } from './MainContainer';
1010
export { default as MainNav } from './MainNav';
11+
export { default as QueryStateUpdater } from './MainNav/QueryStateUpdater';
1112
export { AppOrderProvider } from './MainNav/AppOrderProvider';
1213
export { default as ModuleContainer } from './ModuleContainer';
1314
export { withModule, withModules } from './Modules';

0 commit comments

Comments
 (0)