Skip to content

Rerender only when data for given reduxes has changed #2564

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.

Sign up for GitHub

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

Merged
openshift-merge-robot merged 1 commit into from
Sep 7, 2019
Merged

Rerender only when data for given reduxes has changed #2564

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions frontend/public/components/utils/firehose.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,20 @@ const worstError = errors => {
return worst;
};

const mapStateToProps = ({k8s}) => ({
k8s,
});

const propsAreEqual = (prevProps, nextProps) => {
if (nextProps.children === prevProps.children && nextProps.reduxes === prevProps.reduxes) {
Copy link
Contributor

@christianvogt christianvogt Sep 4, 2019

Choose a reason for hiding this comment

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

There are several other props missing from this check: filters, loaded, loadError, resources

Copy link
Contributor Author

@rawagner rawagner Sep 4, 2019

Choose a reason for hiding this comment

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

how so ? ConnectToState has only 3 props - children, reduxes (both from Firehose) and k8s (from mapStateToProps). Am I wrong ?

Copy link
Contributor

@christianvogt christianvogt Sep 5, 2019

Choose a reason for hiding this comment

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

Sorry you're right. Not sure how I came to that conclusion before.

return nextProps.reduxes.every(({ reduxID }) => prevProps.k8s.get(reduxID) === nextProps.k8s.get(reduxID));
}
return false;
};

// A wrapper Component that takes data out of redux for a list or object at some reduxID ...
// passing it to children
const ConnectToState = connect(({k8s}, {reduxes}) => {
const ConnectToState = connect(mapStateToProps)(React.memo(({ k8s, reduxes, children }) => {
const resources = {};

reduxes.forEach(redux => {
Expand All @@ -84,18 +95,16 @@ const ConnectToState = connect(({k8s}, {reduxes}) => {
const loaded = _.every(resources, resource => (resource.optional ? resource.loaded || !_.isEmpty(resource.loadError) : resource.loaded));
const loadError = worstError(_.map(required, 'loadError').filter(Boolean));

return Object.assign({}, resources, {
const k8sResults = Object.assign({}, resources, {
filters: Object.assign({}, ..._.map(resources, 'filters')),
loaded,
loadError,
reduxIDs: _.map(reduxes, 'reduxID'),
resources,
});
}, null, null, {
areStatesEqual: (next, prev) => (next.k8s === prev.k8s),
})(props =>
inject(props.children, _.omit(props, ['children', 'className', 'reduxes']))
);

return inject(children, k8sResults);
}, propsAreEqual));

const stateToProps = ({k8s}, {resources}) => {
const k8sModels = resources.reduce((models, {kind}) => models.set(kind, k8s.getIn(['RESOURCES', 'models', kind])), ImmutableMap());
Expand Down