Skip to content
This repository was archived by the owner on May 24, 2024. It is now read-only.

Default error message #32

Merged
merged 9 commits into from
Jun 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ Unreleased
1.22.0 - (April 28, 2020)
------------------
### Changed
* Updated errorBoundary to have a default error message when there is none provided.
* Update terra-toolkit dev dependency to 6.0.0

1.21.0 - (April 22, 2020)
Expand Down
5 changes: 4 additions & 1 deletion CONTRIBUTORS.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,10 @@ Cerner Corporation
- Tyler Biethman [@tbiethman]
- David Schoonover [@dkschoonover]
- Dianna McGinn [@DMcginn]
- Jaime Mackey [@jmsv6d]

[@tbiethman]: https://github.com/tbiethman
[@dkschoonover]: https://github.com/dkschoonover
[@DMcginn]: https://github.com/DMcginn
[@DMcginn]: https://github.com/DMcginn
[@jmsv6d]: https://github.com/jmsv6d

15 changes: 12 additions & 3 deletions src/application-error-boundary/ApplicationErrorBoundary.jsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import React from 'react';
import PropTypes from 'prop-types';
import StatusView from 'terra-status-view';
import { injectIntl, intlShape } from 'react-intl';
import logger from '../utils/logger';

const propTypes = {
Expand All @@ -9,6 +10,12 @@ const propTypes = {
* by these components during their render lifecycle will be caught by the ApplicationErrorBoundary.
*/
children: PropTypes.node,

/**
* @private
* Intl object for translations.
*/
intl: PropTypes.shape(intlShape),
};

/**
Expand Down Expand Up @@ -78,14 +85,16 @@ class ApplicationErrorBoundary extends React.Component {
}

render() {
const { children } = this.props;
const { children, intl } = this.props;
const activeError = this.state.error || this.errorRef.current;

if (activeError) {
const errorDetails = activeError.message.toString();
const errorText = intl.formatMessage({ id: 'terraApplication.errorBoundary.defaultErrorMessage' }, { errorDetails });
return (
<StatusView
variant="error"
message={activeError.toString()}
message={errorText}
Copy link
Contributor

Choose a reason for hiding this comment

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

Could I get some more information about why we're changing the error boundary text? What cases are they being used for? I assumed they would be here in the case of major app failures. Are people seeing these a lot?

Copy link
Author

Choose a reason for hiding this comment

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

If there's an error thrown without any error message, then the error view will display the error type without any indication to the user about what went wrong. This would provide some sort of information to the user in that case. This would likely only happen if a custom error was thrown that didn't provide any message and was not caught anywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Have we seen instances where the thrown js exception doesn't have an error message? I don't mean to be difficult but I wondering what brought this change up?

Copy link
Contributor

Choose a reason for hiding this comment

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

@mjhenkes, UX didn't like it. Didn't think it was user friendly

/>
);
}
Expand All @@ -96,4 +105,4 @@ class ApplicationErrorBoundary extends React.Component {

ApplicationErrorBoundary.propTypes = propTypes;

export default ApplicationErrorBoundary;
export default injectIntl(ApplicationErrorBoundary);
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,21 @@ import ApplicationErrorBoundary from '../../../../lib/application-error-boundary

const ErrorComponent = () => {
const [throwError, setThrowError] = useState(false);
const [throwDefaultError, setThrowDefaultError] = useState(false);

if (throwError) {
throw new Error('ouch');
}

if (throwDefaultError) {
throw new Error();
}

return (
<button type="button" onClick={() => { setThrowError(true); }}>Throw Error</button>
<>
<button type="button" onClick={() => { setThrowError(true); }}>Throw Error</button>
<button type="button" id="defaultMessage" onClick={() => { setThrowDefaultError(true); }}>Throw Default Error</button>
</>
);
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ exports[`ApplicationBase should render with all props 1`] = `
</div>
}
>
<ApplicationErrorBoundary>
<InjectIntl(ApplicationErrorBoundary)>
<InjectIntl(Component)>
<ActiveBreakpointProvider>
<withPromptRegistration(NavigationPromptCheckpoint)
Expand All @@ -53,7 +53,7 @@ exports[`ApplicationBase should render with all props 1`] = `
</withPromptRegistration(NavigationPromptCheckpoint)>
</ActiveBreakpointProvider>
</InjectIntl(Component)>
</ApplicationErrorBoundary>
</InjectIntl(ApplicationErrorBoundary)>
</Base>
</ThemeContextProvider>
</ThemeProvider>
Expand All @@ -79,7 +79,7 @@ exports[`ApplicationBase should render with minimal props 1`] = `
locale="en"
strictMode={false}
>
<ApplicationErrorBoundary>
<InjectIntl(ApplicationErrorBoundary)>
<InjectIntl(Component)>
<ActiveBreakpointProvider>
<withPromptRegistration(NavigationPromptCheckpoint)
Expand All @@ -102,7 +102,7 @@ exports[`ApplicationBase should render with minimal props 1`] = `
</withPromptRegistration(NavigationPromptCheckpoint)>
</ActiveBreakpointProvider>
</InjectIntl(Component)>
</ApplicationErrorBoundary>
</InjectIntl(ApplicationErrorBoundary)>
</Base>
</ThemeContextProvider>
</ThemeProvider>
Expand All @@ -128,7 +128,7 @@ exports[`ApplicationBase should render with the preferred browser local 1`] = `
locale="en"
strictMode={false}
>
<ApplicationErrorBoundary>
<InjectIntl(ApplicationErrorBoundary)>
<InjectIntl(Component)>
<ActiveBreakpointProvider>
<withPromptRegistration(NavigationPromptCheckpoint)
Expand All @@ -151,7 +151,7 @@ exports[`ApplicationBase should render with the preferred browser local 1`] = `
</withPromptRegistration(NavigationPromptCheckpoint)>
</ActiveBreakpointProvider>
</InjectIntl(Component)>
</ApplicationErrorBoundary>
</InjectIntl(ApplicationErrorBoundary)>
</Base>
</ThemeContextProvider>
</ThemeProvider>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,19 +1,20 @@
import React from 'react';
import { shallowWithIntl, mountWithIntl } from 'terra-enzyme-intl';
import ApplicationErrorBoundary from '../../../src/application-error-boundary/ApplicationErrorBoundary';
import Logger from '../../../src/utils/logger';

describe('ApplicationErrorBoundary', () => {
describe('Snapshots', () => {
it('should render with minimal props', () => {
const wrapper = shallow((
const wrapper = shallowWithIntl((
<ApplicationErrorBoundary />
));

expect(wrapper).toMatchSnapshot();
});

it('should render with children', () => {
const wrapper = shallow((
const wrapper = shallowWithIntl((
<ApplicationErrorBoundary>
<div>Test child</div>
</ApplicationErrorBoundary>
Expand All @@ -26,11 +27,12 @@ describe('ApplicationErrorBoundary', () => {
const spy = jest.spyOn(Logger, 'error').mockImplementation(() => {});
const ErrorComponent = () => <div />;

const wrapper = shallow((
const wrapper = mountWithIntl((
<ApplicationErrorBoundary>
<ErrorComponent />
</ApplicationErrorBoundary>
));
expect(wrapper).toMatchSnapshot();

/**
* After simulating the error, the error view should be rendered.
Expand Down
Loading