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.

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

Jump to bottom

refactor loading and error #292

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

refactor loading and error #292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
26d91ed
fix: resolve sunrise-sunset test missing props
cbarber Aug 4, 2021
92779fe
chore: disallow console.error in jest tests
cbarber Aug 4, 2021
6ff7073
feat: add react-recompose for HoCs
cbarber Aug 4, 2021
f708f4d
feat: add render-while-loading hoc
cbarber Aug 4, 2021
4b70b7c
feat: add render-while-error hoc
cbarber Aug 4, 2021
fb31a8a
refactor: withFragement should return a function that accepts a compo…
cbarber Aug 4, 2021
ffdcdd2
refactor: replace loading conditions with hoc
cbarber Aug 4, 2021
3582f50
refactor: replace error conditions with hoc
cbarber Aug 4, 2021
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
22 changes: 9 additions & 13 deletions app/javascript/components/bathroom.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,11 @@ import React from 'react';
import PropTypes from 'prop-types';
import gql from 'graphql-tag';
import styled from 'styled-components';
import { compose } from 'react-recompose';
import { WhiteSubTitle, WhiteTitleLarge } from '@components/typography';
import { LoadingMessage, ErrorMessage } from '@messages/default-messages';
import renderWhileError from '@hocs/render-while-error';
import renderWhileLoading from '@hocs/render-while-loading';
import withFragment from './hocs/with-fragment';

export const getBathroomCode = gql`
Expand All @@ -18,14 +21,7 @@ const Column = styled.div`
margin-top: 100px;
`;

export const Bathroom = ({ loading, error, location }) => {
if (loading) {
return <LoadingMessage />;
}
if (error) {
return <ErrorMessage />;
}

export const Bathroom = ({ location }) => {
const { bathroomCode } = location;
if (!bathroomCode) {
return null;
Expand All @@ -42,14 +38,14 @@ Bathroom.propTypes = {
location: PropTypes.shape({
bathroomCode: PropTypes.string,
}),
loading: PropTypes.bool,
error: PropTypes.bool,
};

Bathroom.defaultProps = {
location: {},
loading: true,
error: false,
};

export default withFragment(Bathroom, { location: getBathroomCode });
export default compose(
renderWhileError(ErrorMessage),
bencates marked this conversation as resolved.
Show resolved Hide resolved
renderWhileLoading(LoadingMessage),
Comment on lines +48 to +49
Copy link

Choose a reason for hiding this comment

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

Why do these need to take ErrorMessage and LoadingMessage? Those placeholder components are consistent throughout the application, so I think it'd be cleaner to bake them into the respective HOCs.

Copy link
Collaborator Author

@cbarber cbarber Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good point. My first grep of the usages of if (loading) / if (error) showed some alternative code paths, but those ended up being FaCC's that would be better solved by bubbling up their graphql fragments. I think defaulting the component seems reasonable.

withFragment({ location: getBathroomCode }),
)(Bathroom);
Comment on lines +47 to +51
Copy link

Choose a reason for hiding this comment

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

I'm not a fan of using the compose function here. It's not saving us much in the way of complexity or line count, but it is an extra third-party dependency and an extra concept the reader will have to understand.

Suggested change
export default compose(
renderWhileError(ErrorMessage),
renderWhileLoading(LoadingMessage),
withFragment({ location: getBathroomCode }),
)(Bathroom);
const BathroomWithError = renderWhileError(Bathroom, ErrorMessage);
const BathroomWithErrorAndLoading = renderWhileLoading(BathroomWithError, LoadingMessage);
const BathroomWithErrorLoadingAndFragment = withFragment(BathroomWithErrorAndLoading, { location: getBathroomCode });
export default BathroomWithErrorLoadingAndFragment

Copy link
Collaborator Author

@cbarber cbarber Aug 5, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I disagree. It saves us from extra churn when we want to inject/reorder HoCs e.g. if in your example we switch to loading preceding error I should rename three variables.

25 changes: 9 additions & 16 deletions app/javascript/components/date.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,12 @@ import React from 'react';
import PropTypes from 'prop-types';
import gql from 'graphql-tag';
import styled from 'styled-components';
import { compose } from 'react-recompose';
import { dateForTimezone } from '@lib/datetime';
import { colors, fontSizes, spacing, fonts } from '@lib/theme';
import { LoadingMessage, ErrorMessage } from '@messages/default-messages';
import renderWhileError from '@hocs/render-while-error';
import renderWhileLoading from '@hocs/render-while-loading';
import withFragment from './hocs/with-fragment';

export const getTimeZone = gql`
Expand All @@ -22,17 +25,12 @@ export const DateText = styled.div`

export class Date extends React.Component {
static propTypes = {
loading: PropTypes.bool,
error: PropTypes.bool,
location: PropTypes.shape({
timezone: PropTypes.string,
}).isRequired,
};

static defaultProps = {
loading: true,
error: false,
};
static defaultProps = {};

state = { date: null };

Expand Down Expand Up @@ -66,15 +64,6 @@ export class Date extends React.Component {
};

render() {
const { loading, error } = this.props;
if (loading) {
return <LoadingMessage />;
}

if (error) {
return <ErrorMessage />;
}

const { date } = this.state;
if (!date) {
return null;
Expand All @@ -83,4 +72,8 @@ export class Date extends React.Component {
}
}

export default withFragment(Date, { location: getTimeZone });
export default compose(
renderWhileError(ErrorMessage),
renderWhileLoading(LoadingMessage),
withFragment({ location: getTimeZone }),
)(Date);
17 changes: 17 additions & 0 deletions app/javascript/components/hocs/render-while-error.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import PropTypes from 'prop-types';
import {
branch,
compose,
renderComponent,
setPropTypes,
} from 'react-recompose';

export const renderWhileError = errorComponent =>
compose(
setPropTypes({
error: PropTypes.bool.isRequired,
}),
branch(({ error }) => !!error, renderComponent(errorComponent)),
);
Comment on lines +9 to +15
Copy link

Choose a reason for hiding this comment

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

Again, I think react-compose obscures more than it reveals here

Suggested change
export const renderWhileError = errorComponent =>
compose(
setPropTypes({
error: PropTypes.bool.isRequired,
}),
branch(({ error }) => !!error, renderComponent(errorComponent)),
);
export const renderWhileError = Component, ErrorComponent => {
const ComponentWithError = (error, ...props) => (
error
? <ErrorComponent />
: <Component {...props} )/>
);
ComponentWithError.propTypes({
...Component.propTypes,
error: PropTypes.bool.isRequired,
}
return ComponentWithError;
}

Copy link

Choose a reason for hiding this comment

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

Repeating a comment from above, I think it would be cleaner to import { ErrorComponent } here instead of requiring it to be passed in.


export default renderWhileError;
46 changes: 46 additions & 0 deletions app/javascript/components/hocs/render-while-error.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React from 'react';
import ReactTestRenderer from 'react-test-renderer';
import renderWhileError from '@hocs/render-while-error';

describe('renderWhileError higher order component', () => {
it('it renders the error component if error property is true', () => {
const testErrorComponent = () => {
return 'testErrorComponent rendered';
};
const testBaseComponent = () => {
return 'testBaseComponent rendered';
};
const WrappedComponent = renderWhileError(testErrorComponent)(
testBaseComponent,
);

expect(
ReactTestRenderer.create(<WrappedComponent error />).toJSON(),
).toEqual('testErrorComponent rendered');
});
it('it renders the base component if error property is false', () => {
const testErrorComponent = () => {
return 'testErrorComponent rendered';
};
const testBaseComponent = () => {
return 'testBaseComponent rendered';
};
const WrappedComponent = renderWhileError(testErrorComponent)(
testBaseComponent,
);

expect(
ReactTestRenderer.create(<WrappedComponent error={false} />).toJSON(),
).toEqual('testBaseComponent rendered');
});
it('it has a prop error if error property is undefined', () => {
const noop = () => {};
const WrappedComponent = renderWhileError(noop)(noop);

expect(() => {
ReactTestRenderer.create(<WrappedComponent />).toJSON();
}).toThrow(
'Failed prop type: The prop `error` is marked as required in `branch(noop)`, but its value is `undefined`.',
);
});
});
17 changes: 17 additions & 0 deletions app/javascript/components/hocs/render-while-loading.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
import PropTypes from 'prop-types';
import {
branch,
compose,
renderComponent,
setPropTypes,
} from 'react-recompose';

export const renderWhileLoading = loadingComponent =>
compose(
setPropTypes({
loading: PropTypes.bool.isRequired,
}),
branch(({ loading }) => !!loading, renderComponent(loadingComponent)),
);

export default renderWhileLoading;
46 changes: 46 additions & 0 deletions app/javascript/components/hocs/render-while-loading.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import React from 'react';
import ReactTestRenderer from 'react-test-renderer';
import renderWhileLoading from '@hocs/render-while-loading';

describe('renderWhileLoading higher order component', () => {
it('it renders the loading component if loading property is true', () => {
const testLoadingComponent = () => {
return 'testLoadingComponent rendered';
};
const testBaseComponent = () => {
return 'testBaseComponent rendered';
};
const WrappedComponent = renderWhileLoading(testLoadingComponent)(
testBaseComponent,
);

expect(
ReactTestRenderer.create(<WrappedComponent loading />).toJSON(),
).toEqual('testLoadingComponent rendered');
});
it('it renders the base component if loading property is false', () => {
const testLoadingComponent = () => {
return 'testLoadingComponent rendered';
};
const testBaseComponent = () => {
return 'testBaseComponent rendered';
};
const WrappedComponent = renderWhileLoading(testLoadingComponent)(
testBaseComponent,
);

expect(
ReactTestRenderer.create(<WrappedComponent loading={false} />).toJSON(),
).toEqual('testBaseComponent rendered');
});
it('it has a prop error if loading property is undefined', () => {
const noop = () => {};
const WrappedComponent = renderWhileLoading(noop)(noop);

expect(() => {
ReactTestRenderer.create(<WrappedComponent />).toJSON();
}).toThrow(
'Failed prop type: The prop `loading` is marked as required in `branch(noop)`, but its value is `undefined`.',
);
});
});
6 changes: 4 additions & 2 deletions app/javascript/components/hocs/with-fragment.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { filter } from 'graphql-anywhere';
* withFragment returns a component which expects props that match
* WrappedComponent's fragment names
*/
export default function withFragment(WrappedComponent, fragmentObject) {
export const withFragment = fragmentObject => WrappedComponent => {
const Enhanced = React.memo(props => {
const fragmentKeys = Object.keys(fragmentObject);
const fragmentDataProps = fragmentKeys.reduce((accProps, fragmentKey) => {
Expand All @@ -20,4 +20,6 @@ export default function withFragment(WrappedComponent, fragmentObject) {
});

return Enhanced;
}
};

export default withFragment;
4 changes: 2 additions & 2 deletions app/javascript/components/hocs/with-fragment.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ describe('withFragment higher order component', () => {
});
return 'testComponent rendered';
};
const WrappedComponent = withFragment(testComponent, {
const WrappedComponent = withFragment({
location: locationQuery,
});
})(testComponent);
expect(
ReactTestRenderer.create(
<WrappedComponent location={locationData} />,
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/components/side-panel.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,4 @@ SidePanel.defaultProps = {
error: false,
};

export default withFragment(SidePanel, { location: getSidePanel });
export default withFragment({ location: getSidePanel })(SidePanel);
25 changes: 10 additions & 15 deletions app/javascript/components/time-hero.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,10 @@ import React from 'react';
import PropTypes from 'prop-types';
import gql from 'graphql-tag';
import Time from '@components/time';
import { compose } from 'react-recompose';
import { LoadingMessage, ErrorMessage } from '@messages/default-messages';
import renderWhileError from '@hocs/render-while-error';
import renderWhileLoading from '@hocs/render-while-loading';
import withFragment from './hocs/with-fragment';

export const getTimeHero = gql`
Expand All @@ -11,26 +14,18 @@ export const getTimeHero = gql`
}
`;

export const TimeHero = ({ loading, error, location }) => {
if (loading) {
return <LoadingMessage />;
}
if (error) {
return <ErrorMessage />;
}

export const TimeHero = ({ location }) => {
return <Time location={location} />;
};

TimeHero.propTypes = {
loading: PropTypes.bool,
error: PropTypes.bool,
location: PropTypes.shape({}).isRequired,
};

TimeHero.defaultProps = {
loading: true,
error: false,
};
TimeHero.defaultProps = {};

export default withFragment(TimeHero, { location: getTimeHero });
export default compose(
renderWhileError(ErrorMessage),
renderWhileLoading(LoadingMessage),
withFragment({ location: getTimeHero }),
)(TimeHero);
2 changes: 1 addition & 1 deletion app/javascript/components/top-corner-controller.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,4 +75,4 @@ TopCorner.defaultProps = {
error: false,
};

export default withFragment(TopCorner, { location: getTopCorner });
export default withFragment({ location: getTopCorner })(TopCorner);
2 changes: 1 addition & 1 deletion app/javascript/components/widget-display.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,4 +166,4 @@ export const WidgetDisabledDisplay = () => (
</Wrapper>
);

export default withFragment(WidgetDisplay, { location: getWidgetDisplay });
export default withFragment({ location: getWidgetDisplay })(WidgetDisplay);
2 changes: 1 addition & 1 deletion app/javascript/components/widgets/weather/current-temp.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,4 @@ CurrentTemp.propTypes = {
}).isRequired,
};

export default withFragment(CurrentTemp, { weather: getCurrentTemp });
export default withFragment({ weather: getCurrentTemp })(CurrentTemp);
2 changes: 1 addition & 1 deletion app/javascript/components/widgets/weather/current-weather.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,4 +60,4 @@ CurrentWeather.defaultProps = {
useLargeIcon: false,
};

export default withFragment(CurrentWeather, { weather: getCurrentWeather });
export default withFragment({ weather: getCurrentWeather })(CurrentWeather);
2 changes: 1 addition & 1 deletion app/javascript/components/widgets/weather/daily-weather.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,4 +92,4 @@ DailyWeather.propTypes = {
}).isRequired,
};

export default withFragment(DailyWeather, { weather: getDailyWeather });
export default withFragment({ weather: getDailyWeather })(DailyWeather);
2 changes: 1 addition & 1 deletion app/javascript/components/widgets/weather/hourly-temps.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,4 +98,4 @@ HourlyTemps.defaultProps = {
hours: 5,
};

export default withFragment(HourlyTemps, { weather: getHourlyWeather });
export default withFragment({ weather: getHourlyWeather })(HourlyTemps);
4 changes: 2 additions & 2 deletions app/javascript/components/widgets/weather/sunrise-sunset.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,7 @@ SunriseSunset.propTypes = {
weather: PropTypes.shape({}).isRequired,
};

export default withFragment(SunriseSunset, {
export default withFragment({
weather: getSunriseSunsetWeather,
location: getSunriseSunsetLocation,
});
})(SunriseSunset);
7 changes: 6 additions & 1 deletion app/javascript/components/widgets/weather/sunrise-sunset.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@ import React from 'react';
import ReactTestRenderer from 'react-test-renderer';

const location = {
cityName: 'Providence',
moonPhase: 0.5,
timezone: 'America/New_York',
solarCycles: [
{ type: 'sunset', time: '2021-06-09T00:18:31Z' },
Expand All @@ -11,7 +13,10 @@ const location = {
],
};

const weather = { moonPhase: 1, current: { weather: { id: 803 } } };
const weather = {
moonPhase: 1,
current: { weather: { id: 803 }, windSpeed: 1 },
};

describe('SunriseSunset component', () => {
const mockDate = new Date('2021-06-09T00:18:31Z');
Expand Down
2 changes: 1 addition & 1 deletion app/javascript/components/widgets/weather/weather-effect.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -509,4 +509,4 @@ WeatherEffect.propTypes = {
}).isRequired,
};

export default withFragment(WeatherEffect, { weather: getWeatherEffect });
export default withFragment({ weather: getWeatherEffect })(WeatherEffect);
2 changes: 1 addition & 1 deletion app/javascript/components/widgets/weather/weather-icon.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,4 +26,4 @@ CurrentIcon.propTypes = {
}).isRequired,
};

export default withFragment(CurrentIcon, { weather: getCurrentIcon });
export default withFragment({ weather: getCurrentIcon })(CurrentIcon);
Loading