refactor: error boundary (openedx#8)

* refactor: error boundary --------- Co-authored-by: Maxwell Frank <[email protected]>
davidjoy · Dec 19, 2023 · 9064049 · 9064049
1 parent 9c35176
commit 9064049
Show file tree

Hide file tree

Showing 4 changed files with 36 additions and 39 deletions.
diff --git a/src/plugins/Plugin.jsx b/src/plugins/Plugin.jsx
@@ -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,
@@ -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>
+);
 
+// 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,
@@ -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({
@@ -65,6 +59,7 @@ const Plugin = ({
   }, []);
 
   useEffect(() => {
+    // TODO: find out where "ready" comes from and when it would be true
     if (ready) {
       dispatchReadyEvent();
     }
@@ -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>
@@ -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,
+};
diff --git a/src/plugins/Plugin.test.jsx b/src/plugins/Plugin.test.jsx
@@ -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');
 
@@ -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');
   });
 });
 
@@ -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>
@@ -150,7 +150,7 @@ describe('Plugin', () => {
     </div>
   );
 
-  const errorFallbackComponent = () => (
+  const ErrorFallbackComponent = () => (
     <div>
       <p>
         <FormattedMessage
@@ -166,7 +166,7 @@ describe('Plugin', () => {
   it('should render children if no error', () => {
     const component = (
       <PluginPageWrapper
-        errorFallback={errorFallbackComponent}
+        ErrorFallbackComponent={ErrorFallbackComponent}
         ChildComponent={HealthyComponent}
       />
     );
@@ -178,7 +178,7 @@ describe('Plugin', () => {
     const component = (
       <PluginPageWrapper
         className="bg-light"
-        errorFallback={errorFallbackComponent}
+        ErrorFallbackComponent={ErrorFallbackComponent}
         ChildComponent={ExplodingComponent}
       />
     );
@@ -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}
       />
     );

diff --git a/src/plugins/PluginContainerIframe.jsx b/src/plugins/PluginContainerIframe.jsx
@@ -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',
           { 'd-none': !ready },
           className,
         )}

diff --git a/src/plugins/PluginSlot.jsx b/src/plugins/PluginSlot.jsx
@@ -21,17 +21,14 @@ const PluginSlot = forwardRef(({
   */
   const { plugins, keepDefault } = usePluginSlot(id);
 
-  const { fallback } = pluginProps;
+  const { loadingFallback } = pluginProps;
 
-  // 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;
 
   let finalChildren = [];
   if (plugins.length > 0) {
@@ -43,7 +40,7 @@ const PluginSlot = forwardRef(({
         <PluginContainer
           key={pluginConfig.url}
           config={pluginConfig}
-          fallback={finalFallback}
+          fallback={finalLoadingFallback}
           {...pluginProps}
         />,
       );