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

refactor: error boundary #8

Merged
merged 3 commits into from
Dec 19, 2023
Merged

refactor: error boundary #8

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7fa76d7
refactor: error boundary
MaxFrank13 Dec 15, 2023
3dda7d4
refactor: remove dev comments
MaxFrank13 Dec 18, 2023
93084fe
refactor: fixed tests
MaxFrank13 Dec 18, 2023
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
46 changes: 23 additions & 23 deletions src/plugins/Plugin.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,7 @@ import React, {
useEffect, useMemo, useState,
} from 'react';
import PropTypes from 'prop-types';
import { ErrorBoundary } from 'react-error-boundary';
import { logError } from '@edx/frontend-platform/logging';
import { ErrorBoundary } from '@edx/frontend-platform/react';
import {
injectIntl,
intlShape,
Expand All @@ -17,19 +16,18 @@ import {
import { PLUGIN_RESIZE } from './data/constants';
import messages from './Plugins.messages';

// TODO: see example-plugin-app/src/PluginOne.jsx for example of customizing errorFallback
function errorFallbackDefault(intl) {
return (
<div>
<h2>
{intl.formatMessage(messages.unexpectedError)}
</h2>
</div>
);
}
// TODO: create example-plugin-app/src/PluginOne.jsx for example of customizing errorFallback
const ErrorFallbackDefault = ({ intl }) => (
<div>
<h2>
{intl.formatMessage(messages.unexpectedError)}
</h2>
</div>
);
Comment on lines +20 to +26
Copy link
Member Author

@MaxFrank13 MaxFrank13 Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I changed the errorFallBackProp to ErrorFallbackComponent. It is now capitalized to signify that it is a React component. The reason why I wanted to make this clear is because the <ErrorBoundary /> component from frontend-platform wouldn't render anything unless it was provided as a React Component (ie. <ErrorFallback />). It was picking up on the prop but wasn't rendering it to the page unless it was provided this way.


// TODO: find out where "ready" comes from
const Plugin = ({
children, className, intl, style, ready, errorFallbackProp,
children, className, intl, style, ready, ErrorFallbackComponent,
}) => {
const [dimensions, setDimensions] = useState({
width: null,
Expand All @@ -41,13 +39,9 @@ const Plugin = ({
...style,
}), [dimensions, style]);

const errorFallback = errorFallbackProp || errorFallbackDefault;

// Error logging function
// Need to confirm: When an error is caught here, the logging will be sent to the child MFE's logging service
const logErrorToService = (error, info) => {
logError(error, { stack: info.componentStack });
};

const ErrorFallback = ErrorFallbackComponent || ErrorFallbackDefault;

useHostEvent(PLUGIN_RESIZE, ({ payload }) => {
setDimensions({
Expand All @@ -65,6 +59,7 @@ const Plugin = ({
}, []);

useEffect(() => {
// TODO: find out where "ready" comes from and when it would be true
if (ready) {
dispatchReadyEvent();
}
Expand All @@ -73,8 +68,9 @@ const Plugin = ({
return (
<div className={className} style={finalStyle}>
<ErrorBoundary
FallbackComponent={() => errorFallback(intl)}
onError={logErrorToService}
// Must be React Component format (<ComponentName ...props />) or it won't render
// TODO: update frontend-platform code to refactor <ErrorBoundary /> or include info in docs somewhere
fallbackComponent={<ErrorFallback intl={intl} />}
>
{children}
</ErrorBoundary>
Expand All @@ -87,15 +83,19 @@ export default injectIntl(Plugin);
Plugin.propTypes = {
children: PropTypes.node.isRequired,
className: PropTypes.string,
errorFallbackProp: PropTypes.func,
ErrorFallbackComponent: PropTypes.func,
intl: intlShape.isRequired,
ready: PropTypes.bool,
style: PropTypes.object, // eslint-disable-line
};

Plugin.defaultProps = {
className: null,
errorFallbackProp: null,
ErrorFallbackComponent: null,
style: {},
ready: true,
};

ErrorFallbackDefault.propTypes = {
intl: intlShape.isRequired,
};
16 changes: 8 additions & 8 deletions src/plugins/Plugin.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ describe('PluginContainer', () => {
expect(iframeElement.attributes.getNamedItem('scrolling').value).toEqual('auto');
expect(iframeElement.attributes.getNamedItem('title').value).toEqual(title);
// The component isn't ready, since the class has 'd-none'
expect(iframeElement.attributes.getNamedItem('class').value).toEqual('border border-0 d-none');
expect(iframeElement.attributes.getNamedItem('class').value).toEqual('border border-0 w-100 d-none');

jest.spyOn(iframeElement.contentWindow, 'postMessage');

Expand Down Expand Up @@ -97,7 +97,7 @@ describe('PluginContainer', () => {
readyEvent.source = iframeElement.contentWindow;
fireEvent(window, readyEvent);

expect(iframeElement.attributes.getNamedItem('class').value).toEqual('border border-0');
expect(iframeElement.attributes.getNamedItem('class').value).toEqual('border border-0 w-100');
});
});

Expand Down Expand Up @@ -127,10 +127,10 @@ describe('Plugin', () => {
});

const PluginPageWrapper = ({
params, errorFallback, ChildComponent,
params, ErrorFallbackComponent, ChildComponent,
}) => (
<IntlProvider locale="en">
<Plugin params={params} errorFallbackProp={errorFallback}>
<Plugin params={params} ErrorFallbackComponent={ErrorFallbackComponent}>
<ChildComponent />
</Plugin>
</IntlProvider>
Expand All @@ -150,7 +150,7 @@ describe('Plugin', () => {
</div>
);

const errorFallbackComponent = () => (
const ErrorFallbackComponent = () => (
<div>
<p>
<FormattedMessage
Expand All @@ -166,7 +166,7 @@ describe('Plugin', () => {
it('should render children if no error', () => {
const component = (
<PluginPageWrapper
errorFallback={errorFallbackComponent}
ErrorFallbackComponent={ErrorFallbackComponent}
ChildComponent={HealthyComponent}
/>
);
Expand All @@ -178,7 +178,7 @@ describe('Plugin', () => {
const component = (
<PluginPageWrapper
className="bg-light"
errorFallback={errorFallbackComponent}
ErrorFallbackComponent={ErrorFallbackComponent}
ChildComponent={ExplodingComponent}
/>
);
Expand All @@ -197,7 +197,7 @@ describe('Plugin', () => {
it('should render the passed in fallback component when the error boundary receives a React error', () => {
const component = (
<PluginPageWrapper
errorFallback={errorFallbackComponent}
ErrorFallbackComponent={ErrorFallbackComponent}
ChildComponent={ExplodingComponent}
/>
);
Expand Down
2 changes: 1 addition & 1 deletion src/plugins/PluginContainerIframe.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ const PluginContainerIframe = ({
scrolling={scrolling}
referrerPolicy="origin" // The sent referrer will be limited to the origin of the referring page: its scheme, host, and port.
className={classNames(
'border border-0',
'border border-0 w-100',
Copy link
Member Author

Choose a reason for hiding this comment

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

This sets the width: 100%; CSS rule that enables the iframe to stretch its width to available space. The width is currently being set (I believe) by the useElementSize() hook which sets the width to a fixed number determined by the elements that are wrapped by the plugin

{ 'd-none': !ready },
className,
)}
Expand Down
11 changes: 4 additions & 7 deletions src/plugins/PluginSlot.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,17 +21,14 @@
*/
const { plugins, keepDefault } = usePluginSlot(id);

const { fallback } = pluginProps;
const { loadingFallback } = pluginProps;

Check warning on line 24 in src/plugins/PluginSlot.jsx

View check run for this annotation

Codecov / codecov/patch

src/plugins/PluginSlot.jsx#L24 

Added line #L24 was not covered by tests

// TODO: Add internationalization to the "Loading" text on the spinner.
let finalFallback = (
const defaultLoadingFallback = (
<div className={classNames(pluginProps.className, 'd-flex justify-content-center align-items-center')}>
<Spinner animation="border" screenReaderText={intl.formatMessage(messages.loading)} />
</div>
);
if (fallback !== undefined) {
finalFallback = fallback;
}
const finalLoadingFallback = loadingFallback !== undefined ? loadingFallback : defaultLoadingFallback;
Comment on lines +26 to +31
Copy link
Member Author

@MaxFrank13 MaxFrank13 Dec 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is now loadingFallback to better show what it is used for. This fallback will be rendered the page while the PLUGIN_READY value is false. The PLUGIN_READY event is fired when the Plugin component mounts. This event will never fire if there is no Plugin being wrapped by the PluginSlot . And thus the spinner would remain on the screen with no indication to the viewer.

This is important to note when creating Plugin — perhaps a PluginWrapper component should be encouraged? Similar to what is used in the test files? This ensures that alerts, or any other fallback components for the Plugin, are wrapped and thus will fire the "ready" event which will clear the loading spinner and render whatever fallback is intended. Happy to elaborate more on this


let finalChildren = [];
if (plugins.length > 0) {
Expand All @@ -43,7 +40,7 @@
<PluginContainer
key={pluginConfig.url}
config={pluginConfig}
fallback={finalFallback}
fallback={finalLoadingFallback}
{...pluginProps}
/>,
);
Expand Down