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.

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

feat: ErrorBoundary for PluginContainer #96

Merged
merged 6 commits into from
Nov 1, 2024

Conversation

MaxFrank13
Copy link
Member

@MaxFrank13 MaxFrank13 commented Oct 22, 2024

This adds an ErrorBoundary (from frontend-platform) around each PluginContainer and provides two methods with which consumers can set the fallback component that renders when an error is caught.

Also, the pluginSlots directory of the example app has been renamed to plugin-slots in order to keep with the pattern we have set in MFEs.

PluginSlot props

Set the slotErrorFallbackComponent prop in the PluginSlot to a React component. This will replace the default <ErrorPage /> from frontend-platform.

<PluginSlot
  id='my-plugin-slot'
  slotErrorFallbackComponent={<MyCustomFallbackComponent />}
/>

JS config

Set the errorFallbackComponent field for the specific plugin to the custom fallback component in the JS configuration. This will be prioritized over any other fallback components.

const config = {
  pluginSlots: {
    my_plugin_slot: {
      keepDefault: false,
      plugins: [
        {
          op: PLUGIN_OPERATIONS.Insert,
          widget: {
            id: 'this_is_a_plugin',
            type: DIRECT_PLUGIN,
            priority: 60,
            RenderWidget: ReactPluginComponent,
            errorFallbackComponent: MyCustomFallbackComponent,
          },
        },
      ],
    },
  },
};

Copy link

codecov bot commented Oct 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.81%. Comparing base (db9fb77) to head (9da0e36).
Report is 2 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master      #96   +/-   ##
=======================================
  Coverage   97.80%   97.81%           
=======================================
  Files          10       10           
  Lines         228      229    +1     
  Branches       55       59    +4     
=======================================
+ Hits          223      224    +1     
  Misses          4        4           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MaxFrank13 MaxFrank13 force-pushed the mfrank/plugin-error-boundary branch from fad73ae to c3c816c Compare October 23, 2024 17:25
README.rst Outdated Show resolved Hide resolved
Co-authored-by: Jason Wesson <jsnwesson@gmail.com>
@brian-smith-tcril
Copy link
Contributor

This is a great idea! Do you have examples of slots that you'd like to update to use this?

@MaxFrank13
Copy link
Member Author

MaxFrank13 commented Nov 1, 2024

This is a great idea! Do you have examples of slots that you'd like to update to use this?

I think the course_card_action_slot (source) from Learner Dashboard would be a good candidate. Seeing as the slot is much too small for the ErrorPage fallback from frontend-platform, we'll probably want a fallback that fits the space. That would probably need to go through product review though I assume?

@brian-smith-tcril
Copy link
Contributor

This is a great idea! Do you have examples of slots that you'd like to update to use this?

I think the course_card_action_slot (source) from Learner Dashboard would be a good candidate. Seeing as the slot is much too small for the ErrorPage fallback from frontend-platform, we'll probably want a fallback that fits the space. That would probably need to go through product review though I assume?

Yeah, there are definitely some product questions around that one - one of the main questions is "do we want to have the upgrade button by default?" If the answer to that is "no," then it wouldn't be a great candidate for a slot to show this off with.

I think an ideal candidate would:

  • Have default content
  • Have default error content that isn't ErrorPage
  • Have a clear use case for replacing the default content (as opposed to wrapping or hiding)
  • Have a clear use case for replacing the error content. I think this would make the most sense if the default error content has wording that doesn't make sense outside of the context of the default content.

@MaxFrank13 MaxFrank13 merged commit cb9d251 into master Nov 1, 2024
7 checks passed
@MaxFrank13 MaxFrank13 deleted the mfrank/plugin-error-boundary branch November 1, 2024 15:05
@openedx-semantic-release-bot

🎉 This PR is included in version 1.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MaxFrank13
Copy link
Member Author

one of the main questions is "do we want to have the upgrade button by default?"

My bad! I was conflating internal interests with open source. I blame the fact that it's the end of the week 😅 . We are working on removing the Upgrade button from the open source repo so I agree it's actually not a great candidate.

@brian-smith-tcril
Copy link
Contributor

No worries at all! I definitely agree that is a great example of a place where ErrorPage doesn't make sense, and having a custom fallback there absolutely does! It just doesn't quite check all of the boxes I'd hope a "shining example" would.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants