Skip to content

Render components only after we have collected the Ion props #482

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
tgolen merged 10 commits into from
Sep 15, 2020
Merged

Render components only after we have collected the Ion props #482

Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c43eef8
Dont render a component unless we have attempted to load props
marcaaron Sep 12, 2020
eee4a9e
fix style
marcaaron Sep 12, 2020
7c38c89
Do not make get a promise + add check to componentDidUpdate
marcaaron Sep 14, 2020
f8c01e7
revert some other things
marcaaron Sep 14, 2020
89e0099
use omit
marcaaron Sep 14, 2020
7dbce07
add comment
marcaaron Sep 14, 2020
1c092ca
Fix the update method
marcaaron Sep 14, 2020
976d5a5
add comments
marcaaron Sep 14, 2020
6183981
fix bad english
marcaaron Sep 14, 2020
42f3747
Use chirags trick
marcaaron Sep 14, 2020
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
41 changes: 40 additions & 1 deletion src/components/withIon.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,18 @@ export default function (mapIonToState) {
// this.props. These are stored differently because anytime the props change, the component has to be
// reconnected to Ion with the new props.
this.activeConnectionIDsWithPropsData = {};

this.state = {
loading: true,
};
}

componentDidMount() {
// Subscribe each of the state properties to the proper Ion key
_.each(mapIonToState, (mapping, propertyName) => {
this.connectMappingToIon(mapping, propertyName, this.wrappedComponent);
});
this.checkAndUpdateLoading();
}

componentDidUpdate(prevProps) {
Expand All @@ -55,6 +60,7 @@ export default function (mapIonToState) {
}
}
});
this.checkAndUpdateLoading();
}

componentWillUnmount() {
Expand All @@ -63,6 +69,31 @@ export default function (mapIonToState) {
_.each(this.activeConnectionIDsWithPropsData, Ion.disconnect);
}

/**
* Makes sure each Ion key we requested has been set to state with a value of some kind.
* We are doing this so that the wrapped component will only render when all the data
* it needs is available to it.
*/
checkAndUpdateLoading() {
if (!this.state.loading) {
return;
}

// Filter all keys by those which we do want to init with stored values
// since keys that are configured to not init with stored values will
// never appear on state when the component mounts - only after they update
// organically.
const requiredKeysForInit = _.chain(mapIonToState)
Copy link
Contributor

@chiragsalian chiragsalian Sep 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This logic is a bit confusing with .reduce and prev and curr. If i understand correctly we just want all keys except where initWithStoredValues is defined as false.

Then could this logic be used since it looks easier,

const requiredKeysForInit = _.chain(mapIonToState)
    .omit(value => value.initWithStoredValues === false)
    .keys()
    .value();

Copy link
Contributor

@chiragsalian chiragsalian Sep 14, 2020

Choose a reason for hiding this comment

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

Tested the same,
image

Notice i just got key "myPersonalDetails" and "network" but didn't get "testSomeKey" since it had initWithStoredValues as false.

.omit(config => config.initWithStoredValues === false)
.keys()
.value();

// All state keys should exist and at least have a value of null
if (_.every(requiredKeysForInit, key => !_.isUndefined(this.state[key]))) {
this.setState({loading: false});
}
}

/**
* Takes a single mapping and binds the state of the component to the store
*
Expand Down Expand Up @@ -107,13 +138,21 @@ export default function (mapIonToState) {
}

render() {
if (this.state.loading) {
return null;
}

// Remove any internal state properties used by withIon
// that should not be passed to a wrapped component
const stateToPass = _.omit(this.state, 'loading');

// Spreading props and state is necessary in an HOC where the data cannot be predicted
return (
<WrappedComponent
// eslint-disable-next-line react/jsx-props-no-spreading
{...this.props}
// eslint-disable-next-line react/jsx-props-no-spreading
{...this.state}
{...stateToPass}
/>
);
}
Expand Down