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

[CCR] Refactor redux for Auto-follow pattern detail panel #27491

Merged
merged 10 commits into from
Dec 21, 2018
Original file line number Diff line number Diff line change
Expand Up @@ -7,20 +7,27 @@
import { connect } from 'react-redux';

import { SECTIONS } from '../../constants';
import { getApiStatus, getApiError, getSelectedAutoFollowPattern } from '../../store/selectors';
import { getAutoFollowPattern, saveAutoFollowPattern, clearApiError } from '../../store/actions';
import {
getApiStatus,
getApiError,
getSelectedAutoFollowPatternId,
getSelectedAutoFollowPattern,
} from '../../store/selectors';
import { getAutoFollowPattern, saveAutoFollowPattern, selectEditAutoFollowPattern, clearApiError } from '../../store/actions';
import { AutoFollowPatternEdit as AutoFollowPatternEditView } from './auto_follow_pattern_edit';

const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => ({
apiStatus: getApiStatus(scope)(state),
apiError: getApiError(scope)(state),
autoFollowPattern: getSelectedAutoFollowPattern(state),
autoFollowPatternId: getSelectedAutoFollowPatternId('edit')(state),
autoFollowPattern: getSelectedAutoFollowPattern('edit')(state),
});

const mapDispatchToProps = dispatch => ({
getAutoFollowPattern: (id) => dispatch(getAutoFollowPattern(id)),
selectAutoFollowPattern: (id) => dispatch(selectEditAutoFollowPattern(id)),
saveAutoFollowPattern: (id, autoFollowPattern) => dispatch(saveAutoFollowPattern(id, autoFollowPattern, true)),
clearApiError: () => dispatch(clearApiError(scope)),
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,22 +37,40 @@ export const AutoFollowPatternEdit = injectI18n(
class extends PureComponent {
static propTypes = {
getAutoFollowPattern: PropTypes.func.isRequired,
selectAutoFollowPattern: PropTypes.func.isRequired,
saveAutoFollowPattern: PropTypes.func.isRequired,
clearApiError: PropTypes.func.isRequired,
apiError: PropTypes.object,
apiStatus: PropTypes.string.isRequired,
autoFollowPattern: PropTypes.object,
autoFollowPatternId: PropTypes.string,
}

componentDidMount() {
const { autoFollowPattern, match: { params: { id } } } = this.props;
if (!autoFollowPattern) {
const decodedId = decodeURIComponent(id);
this.props.getAutoFollowPattern(decodedId);
static getDerivedStateFromProps({ autoFollowPatternId }, { lastAutoFollowPatternId }) {
if (lastAutoFollowPatternId !== autoFollowPatternId) {
return { lastAutoFollowPatternId: autoFollowPatternId };
}
}

state = { lastAutoFollowPatternId: undefined }

componentDidMount() {
const { match: { params: { id } }, selectAutoFollowPattern } = this.props;
const decodedId = decodeURIComponent(id);

selectAutoFollowPattern(decodedId);

chrome.breadcrumbs.set([ MANAGEMENT_BREADCRUMB, listBreadcrumb, editBreadcrumb ]);
}

componentDidUpdate(prevProps, prevState) {
const { autoFollowPattern, getAutoFollowPattern } = this.props;
if (!autoFollowPattern && prevState.lastAutoFollowPatternId !== this.state.lastAutoFollowPatternId) {
// Fetch the auto-follow pattern on the server
getAutoFollowPattern(this.state.lastAutoFollowPatternId);
}
}

componentWillUnmount() {
this.props.clearApiError();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,36 +9,30 @@ import { connect } from 'react-redux';
import { SECTIONS } from '../../../constants';
import {
getListAutoFollowPatterns,
getSelectedAutoFollowPatternId,
getApiStatus,
getApiError,
isApiAuthorized,
isAutoFollowPatternDetailPanelOpen as isDetailPanelOpen,
} from '../../../store/selectors';
import {
loadAutoFollowPatterns,
openAutoFollowPatternDetailPanel as openDetailPanel,
closeAutoFollowPatternDetailPanel as closeDetailPanel,
selectDetailAutoFollowPattern,
} from '../../../store/actions';
import { AutoFollowPatternList as AutoFollowPatternListView } from './auto_follow_pattern_list';

const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => ({
autoFollowPatterns: getListAutoFollowPatterns(state),
autoFollowPatternId: getSelectedAutoFollowPatternId('detail')(state),
apiStatus: getApiStatus(scope)(state),
apiError: getApiError(scope)(state),
isAuthorized: isApiAuthorized(scope)(state),
isDetailPanelOpen: isDetailPanelOpen(state),
});

const mapDispatchToProps = dispatch => ({
loadAutoFollowPatterns: (inBackground) => dispatch(loadAutoFollowPatterns(inBackground)),
openDetailPanel: (name) => {
dispatch(openDetailPanel(name));
},
closeDetailPanel: () => {
dispatch(closeDetailPanel());
},
selectAutoFollowPattern: (id) => dispatch(selectDetailAutoFollowPattern(id)),
});

export const AutoFollowPatternList = connect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,49 +17,71 @@ import { AutoFollowPatternTable, DetailPanel } from './components';

const REFRESH_RATE_MS = 30000;

const getQueryParamPattern = ({ location: { search } }) => {
const { pattern } = extractQueryParams(search);
return pattern ? decodeURIComponent(pattern) : null;
};

export const AutoFollowPatternList = injectI18n(
class extends PureComponent {
static propTypes = {
loadAutoFollowPatterns: PropTypes.func,
selectAutoFollowPattern: PropTypes.func,
autoFollowPatterns: PropTypes.array,
apiStatus: PropTypes.string,
apiError: PropTypes.object,
openDetailPanel: PropTypes.func.isRequired,
closeDetailPanel: PropTypes.func.isRequired,
isDetailPanelOpen: PropTypes.bool,
}

static getDerivedStateFromProps({ autoFollowPatternId }, { lastAutoFollowPatternId }) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just a personal DX anecdote... as I read this code, I found myself having to trace the logic through three lifecycle methods: componentDidMount, getDerivedStateFromProps, and componentDidUpdate. I then had to also factor in what would happen when the user clicks a row (a new pattern is selected, triggering getDerivedStateFromProps). This took me a few minutes to wrap my head around, and then I was still left wondering why we're storing lastAutoFollowPatternId. Through experimentation by removing the logic it supports, I found that its purpose is to prevent an infinite render loop.

To be honest, I feel like this code has become more complex with this change. What is the benefit this complexity brings us?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ComponentDidMount is pretty clear I think, it mounts and checks for the presence of a pattern in the query params. I understand what u say about the 2 other lifecycle methods, I will see how this could be improved.
There is never a benefit when complexity comes in 😊 I thought that using a middleware to change a URL on a single page was also a bit complex, mainly because you had to remember that it existed and that it was there that the search params were being changed. So I tried to move the logic as close as possible to the page (section) component.

if (autoFollowPatternId !== lastAutoFollowPatternId) {
return {
lastAutoFollowPatternId: autoFollowPatternId,
isDetailPanelOpen: !!autoFollowPatternId,
};
}
return null;
}

state = {
lastAutoFollowPatternId: null,
isDetailPanelOpen: false,
};

componentDidMount() {
this.props.loadAutoFollowPatterns();
const { loadAutoFollowPatterns, selectAutoFollowPattern, history } = this.props;

loadAutoFollowPatterns();

// Select the pattern in the URL query params
selectAutoFollowPattern(getQueryParamPattern(history));

// Interval to load auto-follow patterns in the background passing "true" to the fetch method
this.interval = setInterval(() => this.props.loadAutoFollowPatterns(true), REFRESH_RATE_MS);
this.interval = setInterval(() => loadAutoFollowPatterns(true), REFRESH_RATE_MS);
}

componentWillUnmount() {
clearInterval(this.interval);
componentDidUpdate(prevProps, prevState) {
const { history } = this.props;
const { lastAutoFollowPatternId } = this.state;

/**
* Each time our state is updated (through getDerivedStateFromProps())
* we persist the auto-follow pattern id to query params for deep linking
*/
if (lastAutoFollowPatternId !== prevState.lastAutoFollowPatternId) {
if(!lastAutoFollowPatternId) {
history.replace({
search: '',
});
} else {
history.replace({
search: `?pattern=${encodeURIComponent(lastAutoFollowPatternId)}`,
});
}
}
}

componentDidUpdate() {
const {
openDetailPanel,
closeDetailPanel,
isDetailPanelOpen,
history: {
location: {
search,
},
},
} = this.props;

const { pattern: patternName } = extractQueryParams(search);

// Show deeplinked auto follow pattern whenever patterns get loaded or the URL changes.
if (patternName != null) {
openDetailPanel(patternName);
} else if (isDetailPanelOpen) {
closeDetailPanel();
}
componentWillUnmount() {
clearInterval(this.interval);
}

renderEmpty() {
Expand Down Expand Up @@ -102,7 +124,13 @@ export const AutoFollowPatternList = injectI18n(
}

renderList() {
const { autoFollowPatterns, apiStatus } = this.props;
const {
selectAutoFollowPattern,
autoFollowPatterns,
apiStatus,
} = this.props;

const { isDetailPanelOpen } = this.state;

if (apiStatus === API_STATUS.LOADING) {
return (
Expand All @@ -118,7 +146,7 @@ export const AutoFollowPatternList = injectI18n(
return (
<Fragment>
<AutoFollowPatternTable autoFollowPatterns={autoFollowPatterns} />
<DetailPanel />
{isDetailPanelOpen && <DetailPanel closeDetailPanel={() => selectAutoFollowPattern(null)} />}
</Fragment>
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,7 @@
import { connect } from 'react-redux';

import { SECTIONS } from '../../../../../constants';
import {
editAutoFollowPattern,
openAutoFollowPatternDetailPanel as openDetailPanel,
} from '../../../../../store/actions';
import { selectDetailAutoFollowPattern } from '../../../../../store/actions';
import { getApiStatus } from '../../../../../store/selectors';
import { AutoFollowPatternTable as AutoFollowPatternTableComponent } from './auto_follow_pattern_table';

Expand All @@ -21,10 +18,7 @@ const mapStateToProps = (state) => ({
});

const mapDispatchToProps = (dispatch) => ({
editAutoFollowPattern: (name) => dispatch(editAutoFollowPattern(name)),
openDetailPanel: (name) => {
dispatch(openDetailPanel(name));
},
selectAutoFollowPattern: (name) => dispatch(selectDetailAutoFollowPattern(name)),
});

export const AutoFollowPatternTable = connect(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ export const AutoFollowPatternTable = injectI18n(
class extends PureComponent {
static propTypes = {
autoFollowPatterns: PropTypes.array,
openDetailPanel: PropTypes.func.isRequired,
selectAutoFollowPattern: PropTypes.func.isRequired,
}

state = {
Expand Down Expand Up @@ -61,7 +61,7 @@ export const AutoFollowPatternTable = injectI18n(
};

getTableColumns() {
const { intl, editAutoFollowPattern, openDetailPanel } = this.props;
const { intl, selectAutoFollowPattern } = this.props;

return [{
field: 'name',
Expand All @@ -73,7 +73,7 @@ export const AutoFollowPatternTable = injectI18n(
truncateText: false,
render: (name) => {
return (
<EuiLink onClick={() => openDetailPanel(name)}>
<EuiLink onClick={() => selectAutoFollowPattern(name)}>
{name}
</EuiLink>
);
Expand Down Expand Up @@ -142,20 +142,25 @@ export const AutoFollowPatternTable = injectI18n(
},
},
{
name: intl.formatMessage({
id: 'xpack.crossClusterReplication.editIndexPattern.fields.table.actionEditLabel',
defaultMessage: 'Edit',
}),
description: intl.formatMessage({
id: 'xpack.crossClusterReplication.editIndexPattern.fields.table.actionEditDescription',
defaultMessage: 'Edit',
}),
icon: 'pencil',
onClick: ({ name }) => {
editAutoFollowPattern(name);
routing.navigate(encodeURI(`/auto_follow_patterns/edit/${encodeURIComponent(name)}`));
render: ({ name }) => {
const label = i18n.translate('xpack.crossClusterReplication.autoFollowPatternList.table.actionEditDescription', {
defaultMessage: 'Edit auto-follow pattern',
});

return (
<EuiToolTip
content={label}
delay="long"
>
<EuiButtonIcon
aria-label={label}
iconType="pencil"
color="primary"
href={routing.getAutoFollowPatternPath(name)}
/>
</EuiToolTip>
);
},
type: 'icon',
},
],
width: '100px',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,17 @@
import { connect } from 'react-redux';
import { DetailPanel as DetailPanelView } from './detail_panel';

import {
getDetailPanelAutoFollowPattern,
getDetailPanelAutoFollowPatternName,
getApiStatus,
isAutoFollowPatternDetailPanelOpen as isDetailPanelOpen,
} from '../../../../../store/selectors';

import {
closeAutoFollowPatternDetailPanel as closeDetailPanel,
editAutoFollowPattern,
} from '../../../../../store/actions';

import {
SECTIONS
} from '../../../../../constants';
import { getSelectedAutoFollowPattern, getSelectedAutoFollowPatternId, getApiStatus, } from '../../../../../store/selectors';
import { SECTIONS } from '../../../../../constants';

const scope = SECTIONS.AUTO_FOLLOW_PATTERN;

const mapStateToProps = (state) => {
return {
isDetailPanelOpen: isDetailPanelOpen(state),
autoFollowPattern: getDetailPanelAutoFollowPattern(state),
autoFollowPatternName: getDetailPanelAutoFollowPatternName(state),
apiStatus: getApiStatus(scope)(state),
};
};

const mapDispatchToProps = (dispatch) => {
return {
closeDetailPanel: () => {
dispatch(closeDetailPanel());
},
editAutoFollowPattern: (name) => {
dispatch(editAutoFollowPattern(name));
}
};
};
const mapStateToProps = (state) => ({
autoFollowPatternId: getSelectedAutoFollowPatternId('detail')(state),
autoFollowPattern: getSelectedAutoFollowPattern('detail')(state),
apiStatus: getApiStatus(scope)(state),
});

export const DetailPanel = connect(
mapStateToProps,
mapDispatchToProps
)(DetailPanelView);
Loading