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

docs: Add ADR for handling frontend plugin fallback #541

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

Conversation

jsnwesson
Copy link

Description:

This ADR is to propose how we should handle the scenario where the iFrame plugin fails to render.
The decision here will help with releasing the PR to make iFrame plugins possible in MFEs. (#235)

@codecov
Copy link

codecov bot commented Aug 18, 2023

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (3956276) 83.05% compared to head (4c78b7f) 83.17%.
Report is 24 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #541      +/-   ##
==========================================
+ Coverage   83.05%   83.17%   +0.12%     
==========================================
  Files          40       40              
  Lines        1062     1070       +8     
  Branches      195      195              
==========================================
+ Hits          882      890       +8     
  Misses        168      168              
  Partials       12       12              

see 4 files with indirect coverage changes

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


Decision
--------
For the above reasons, we will have failing iFrame plugins return an H1 tag that
Copy link
Author

Choose a reason for hiding this comment

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

I'm fully aware that this is likely not the best way to deliver an error, but in hopes of stirring enough of the pot to bring people into the conversation, I'll leave it here (see Constructive Controversy)

docs/decisions/0008-frontend-plugin-fallback.rst Outdated Show resolved Hide resolved
Comment on lines +24 to +25
says "There was an error.". By doing so, it is up to the engineer/team that implements the plugin
to handle the desired fallback however they would like.
Copy link
Contributor

Choose a reason for hiding this comment

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

How will we do that? By checking for the element returned by the pluginSlot?

Choose a reason for hiding this comment

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

How we will be able to check the element returned by the pluginSlot?

Copy link
Author

Choose a reason for hiding this comment

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

hey all! huge apologies for the delay on these responses. After having written this ADR draft, I realized that the PR I was taking on wasn't in any state for me to confidently know what worked and thus a lack of confidence in the ways we could handle the fallback.

I will be editing this draft to reflect my findings and we'll ideally have a clearer picture to work off of.


Decision
--------
For the above reasons, we will have failing iFrame plugins return an H1 tag that
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we log this in a way that the teams can determine how often this fails for their host site?

Choose a reason for hiding this comment

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

How will the host MFE know that the iFrame has failed to render?

Given that this is a new feature that is being made available to the frontend, we
can't anticipate all of the ways that the plugin will be used for a given repo, nor do we
have any intention to limit the scope of how it can be used. Because we don't all of the use
cases for the plugin, we also don't know the desired fallback method that is desired for any
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cases for the plugin, we also don't know the desired fallback method that is desired for any
cases for the plugin, we also don't know the desired fallback method for any


Decision
--------
For the above reasons, we will have failing iFrame plugins return an H1 tag that
Copy link
Member

Choose a reason for hiding this comment

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

[curious] Is there an opportunity to use an ErrorBoundary in some way here? Either the one defined/used by @edx/frontend-platform or a custom one for this use case.

Copy link
Author

Choose a reason for hiding this comment

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

Ah! You know, I should update this to name that there is a PluginErrorBoundary component that is part of the PR I'm working on for this.

In short, the PluginErrorBoundary will capture any errors that occur within, and whatever decision is made here on how to best reflect an error will be handled by the error boundary.

Copy link
Author

Choose a reason for hiding this comment

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

@Mashal-m
Copy link
Contributor

Hey, @jsnwesson, could you please pass the failed ci check?

@abdullahwaheed abdullahwaheed added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants