-
Notifications
You must be signed in to change notification settings - Fork 111
feat: Add slot for whole application #1799
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
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @xitij2000! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Thanks for the pull request, @xitij2000! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
a7dfa1b
to
cf570dc
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1799 +/- ##
==========================================
- Coverage 93.54% 93.53% -0.02%
==========================================
Files 1136 1137 +1
Lines 23205 23208 +3
Branches 5000 5000
==========================================
Hits 21708 21708
- Misses 1421 1424 +3
Partials 76 76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Adds a slot wrapping the entire application.
cf570dc
to
1eb47c7
Compare
Is it possible to implement this without wrapping the entire application in a |
The context provider still needs to wrap all the components that need to share data, and they could be placed anywhere, so it needs to be as high up in the chain as possible. I also feel something like this could be implemented in frontend-platform instead so it's available to all MFEs. One way would be to just add the wrapper to frontend-platform, and another would be to add another plugin mechanism for plugins to provide additional context wrappers. |
Wrapping an entire application (either in an individual MFE or within That being said, I understand the need for plugins to be able to share information. One reasonable path forward would be to:
|
Hello @brian-smith-tcril @xitij2000 do you think we can wrap this up before teak code freeze(April 24th)? cc: @crathbun428 @bmtcril |
I think the The hookstate approach relies on an external package, but has fewer of these issues, so might be worth exploring as well. @farhaanbukhsh For now would it make sense to move this to hookstate to avoid blocking on this? I can make a quick PR wth the change. |
@xitij2000 yup I think we should go with the hookstate solution to get this working and then we can have a ticket to add an ADR if we have the budget and time for it. |
Description
Adds a slot wrapping the entire application.
While the first two slots have pretty obvious use-cases the third has a use-case that propped up while trying to implement a way to have one plugin slot react to another. For instance, to inject a button in one slot that causes an action in another slot.
If developing code in the main application this would be through some type of shared state, for instance, using redux, or a context manager. Using redux from a plugin is a complex task and would require injecting a slice somehow, which doesn't fit in with the PluginSlot mechanism.
What we can do with a correctly-placed plugin-slot though, is to wrap in the entire app inside a context manager that can carry context for the plugin. That is the approach we've gone with here.
If this approach seems reasonable, it might make sense to have something similar for all MFEs.
An alternate approach is to use a package like: https://hookstate.js.org/docs/global-state/ (as recommended by @farhaanbukhsh) This would allow sharing state without needing to wrap the application; however, it introduces a new package as part of a plugin and that can introduce hard-to-resolve conflicts. If this approach is preferable, then it might make sense to have the package as part of the platform so conflicts can be avoided.