-
Notifications
You must be signed in to change notification settings - Fork 33
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
OEP-65: Composable Micro-frontends #575
Conversation
This commit repurposes OEP-65 to propose using module federation as an architectural approach to solve our frontend composability issues. This stands in contrast to the previous approach for OEP-65, which was to use Piral.
Thanks for the pull request, @davidjoy! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
Standing on the shoulders of giants over here; lets not forget it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of initial comments. Looks great, thanks a lot!
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
…lity.rst Co-authored-by: Adolfo R. Brandes <arbrandes@arbrand.es>
cced05c
to
c07b2d0
Compare
Adding another use case to the Composability section.
- Adding an arbiter. - Light editing for punctuation and clarity - Adding another use case for composability. - Adding build-time package overrides as a composability option. - Adding more details to the reference implementation section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize that this OEP is not in the review period, yet. Still, given that we are having multiple conversations at the same time and they all seem to be tightly connected, I thought I could add my comments right now.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall the proposal is looking great! I left quite a few comments (mostly in places where I felt a bit of clarification could help), but I'm 100% on board with the architectural direction of this!
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
- Rewriting and clarifying the Specification section. Simplifying language around Webpack module federation and adding a variety of links out to external resources. - Adding specific recommendations for Maintaining Dependency Consistency. Also adding it to the Rationale. - Rewriting the section on why "build time" and "dependency maintenance" aren't improved by adding shared dependencies. - Adding monorepos to the Rejected Alternatives section. - Adding a sub-section on Proposed MFE Architecture to the Reference Implementation section.
Hi all - I've done another pass on the OEP. Please see the changelog at the bottom of the document for details. The rewrite of a few of the sections that needed the most work has wiped the comments above from the diff. |
Adding some other helpful arrows.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
Shell MFE | ||
--------- | ||
|
||
We will create a new "shell" MFE to act as the top-level host for all other MFEs. It is exclusively responsible for: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[suggestion/clarifcation] Is this really "all other MFEs" or all other MFEs within a related domain? For example, the Enterprise MFEs are likely not relevant to include in a non-Enterprise domain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is envisioned as one shell for everything across all domains.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about the Course Authoring MFE? Though the "library" functionality is currently separate, it's planned to be merged in, so all of "Studio" is essentially just a single MFE (frontend-app-course-authoring). I don't think it needs a shell or composability, unless there is a future plan to break it up.
Plus, it has a different header and often doesn't need to be themed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I believe the studio header is already in frontend-component-header, for what it's worth. 😄
Looking through the motivation section, I think nearly all of the benefits of a unified shell still apply if course-authoring shares a shell with everything else and get worse if it doesn't. Course teams, instructors, course authors, developers, and operators still see all the same improvements as they would for any other MFE. It's just learners that don't really care about where course-authoring lives since they don't visit it.
I agree that this is generally less important for course-authoring for the reasons you said, but I can't think of an upside to separating it out, rather than just keeping things simpler and folding it in with all the others. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As an aside, I believe the studio header is already in frontend-component-header, for what it's worth. 😄
Yes, and it's currently getting bundled into every MFE, even the learner-only ones :p
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth thinking about how we want headers to work going forward. Thinking out loud - the shell could own the headers in the core product and expose them as modules so that they only get downloaded on demand. That way if (for instance) the studio header is in there, users don't pay the price for it unless they actually visit studio.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would have expected the studio header to get tree-shaken out if it wasn't used by the MFE! if it's not that seems broken. But I digress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I would have expected the studio header to get tree-shaken out if it wasn't used by the MFE!
I explained that in the updated README:
I believe this is because [Studio header] uses
ensureConfig()
at import time, so it cannot be tree-shaken out.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
- Extend the Webpack configuration in the MFEs by defining what modules each "guest" MFE exports. We suggest that the package.json `exports <https://nodejs.org/api/packages.html#subpath-exports>`_ field be used to codify this list of exports, and that Webpack pull it in from package.json to configure ``ModuleFederationPlugin``. The format appears to be the same. | ||
- Give "guest" MFEs a way of seeing their own config, since they'll be getting ``@edx/frontend-platform`` as a shared dependency from the shell, and won't be initializing it themselves. | ||
|
||
Secondary concerns include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[curious/clarification] Are there any details to include in this OEP around routing concerns, beyond just the earlier mention of the react-router
and react-router-dom
packages as shared dependencies? As is, it appears it's up to the "host"/"guest" applications to implement their own code splitting by route rather than getting code splitting out of the box like we would have with a single-spa or Piral (e.g., the "host" would be responsible for loading "guest" applications when the host's route changes).
In the current proposal, it would seem the "host" would need to manually implement its own strategy with React.lazy
and Suspense
or loadable-components
(docs), unless there's an abstraction intended to create a custom wrapper (similar to existing MFE frameworks) that helps with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was envisioning us adding a ModuleRoute
(or a better name if we have one) to frontend-platform that pairs a route path with a particular module to load when it becomes active. I think we could encapsulate the module loading/lazy/Suspense code so that folks don't have to think about it and can just think it routes. The 'shell' will need to use a bunch of these ModuleRoute
s for the "core" modules it expects to load, plus any other routes that we configure for custom MFEs.
One thing I need to go sanity check in a POC is how react-router works with module federation and if there are somehow issues there. Deep linking may get interesting, for instance. 😬 And I'm not sure I understand how we get code splitting "out of the box" with single-spa or Piral, or how they'd be any different... you still need to dictate where your dynamic module boundaries are, presumably. Do you mean that they have code that translates a route into a module? (if so, yeah, we don't get that OOTB)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And now I need to go read up on loadable-components
cause I've never heard of it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I need to go sanity check in a POC is how react-router works with module federation and if there are somehow issues there.
In my early POC's with module federation from awhile ago, I recall routing being a concern; specifically with the sharing of a single instance of history
. For example, if a child MFE ends up with a different history
object than the parent/host MFE, any routing triggered from the child MFE may not work properly.
Hopefully the singleton approach with frontend-platform will mitigate that, though.
- The default configuration for loading "core" MFEs. | ||
- Documentation on how to do development | ||
- A decision on whether we use the MFE config API, env.config.js, both, or something else to supply the module federation configuration, whether it's one big combined document or whether each MFE has its own. | ||
- How we sandbox and put error boundaries around dynamically loaded modules. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[inform] There is a related issue with code splitting today in some Open edX micro-frontends (e.g., frontend-app-learning, frontend-app-learner-portal-enterprise) where new deployments of MFEs (assuming atomic deploys, where all assets from previous deploys are deleted upon deployment) cause ChunkLoadErrors
for users who have the application loaded in the browser and try to dynamically load a chunk that no longer exists from the previous build.
[context] Now that frontend-app-learner-portal-enterprise
recently starting code splitting on each of its ~15-20 page routes with react-router-dom
's Route
component's lazy
prop, we have seen an increase in ChunkLoadErrors
. We are handling it in the short term with a "A new version was released. Please refresh to leverage new features and improvements." type message with a CTA to refresh.
If there is no similar built-in handling for ChunkLoadErrors
, might require some additional documentation around other approaches that MFEs could mitigate the issue (e.g., keeping around the assets of previous builds for a period of time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd expect any built-in error handling around it to be fairly rudimentary, and wouldn't result in finding the most recent version of that chunk. Seems like some companion documentation about best practices when deploying MFEs. Using module federation will likely exacerbate the problem, but it's also already something we're dealing with.
|
||
We need to ensure we minimize breaking changes in our own libraries (such as Paragon, the header, footer, frontend-platform, frontend-build, etc.) We suggest accomplishing this by: | ||
|
||
- Creating new versions of components with breaking changes (``ButtonV2``, ``webpack.dev.config.v2.js``) rather than modifying existing ones. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question/clarification] Do you have any resources/examples (e.g., from other projects) where this approach of creating new versions of components with breaking changes versus modifying existing ones is used?
- Will old versions of components/files will continue to be maintained/supported?
- What guidance is there regarding when old versions of components/files could be removed?
- If a new
ButtonV2
component is introduced (e.g., presumably in@openedx/paragon
), how would consumers actually begin using theButtonV2
version without introducing a breaking change? Unless this is suggesting Paragon would export bothButton
andButtonV2
for consumers to decide which to use? Would thisButtonV2
version ever be renamed back toButton
? From a developer experience POV (as a consumer of Paragon), I would prefer to keep the new/latest version still associated with the standard named export (i.e.,Button
) versus needing to import/useButtonV2
. The current paradigm in Paragon for deprecating components generally introduces a compound component (e.g.,Button.Deprecated
) instead of introducing a version component name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, I'd defer to you on details of the right arrangement here/something that isn't at odds with Paragon's organization philosophy. From what you said, though, I expect the need to almost completely avoid breaking changes may be fundamentally at odds with the current naming scheme.
The end goal is to avoid breaking changes, which means that MFEs that haven't been upgraded need to have a consistent set of exports/imports that don't change out from underneath them. So replacing the implementation of Button
with a new one is a breaking change, but creating a new component called ButtonV2
that maintainers can switch over to at their leisure isn't.
Just thinking out loud, and while acknowledging to the overhead involved - if we need to deprecate old versions of components (which should be pretty infrequent, I'd hope!?), we should follow the DEPR process and align those deprecations to Open edX releases. 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should follow the DEPR process and align those deprecations to Open edX releases. 🤔
💟
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the DEPR process is part of the answer, here, indeed. But I'm not sure it should align with Open edX releases, because that would imply that it's ok to make a breaking change as long as the next release is far enough away. That's not necessarily true.
I believe Paragon should (continue to) exist independently as an upstream library, to which Open edX is a downstream. And as any well-behaved upstream, Paragon should avoid making breaking changes as a matter of adoption: if people get fed up with things breaking all the time, they'll start looking for another library. :P
It just so happens that we are also the maintainers of Paragon, but that doesn't mean we should give ourselves permission to break things often and without serious consideration.
This doesn't mean we can't do it. Just that we should follow certain principles. For instance, whenever it becomes inevitable to release a new major version:
- A deprecation period should be established for the modified behavior (this may or may not imply a
ButtonV2
in the deprecation period) - We should document what breaks very well, and how to migrate to the new version
- We should support the previous release(s) with bug and security fixes to stable branches
Additionally, we could adopt an stable-unstable release strategy like many projects do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One clarification:
But I'm not sure it should align with Open edX releases, because that would imply that it's ok to make a breaking change as long as the next release is far enough away.
I actually meant the opposite. And to be clear, I don't have a lot of experience running our release processes, so this may be untenable for all I know. But I was thinking a new release is an opportunity to make breaking changes since folks are expecting a upgrade process anyway. Like, as part of cutting and finalizing a release, we resolve things like "ButtonV2" back to "Button" and remove the old version/move it to Deprecated, whatever. That's what I meant by "aligning" the DEPR processes to releases, which I thought generally happened anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's how I was interpreting it. Like this timeline, with a potential addition of a third release if a slower timeline is required:
- Sumac introduces ButtonV2 as the new hotness, and default components migrated over. Noted that Button will be deprecated in Teak, but is available for now.
- Teak: ButtonV2 no longer available
Is that what you were thinking?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More or less. I'm not sure how granular these steps need to be, but something like the following. Note that this sounds involved, but it's important to remember we should do this very infrequently as a last resort. And also it sounds like a lot of work for a button, but that's just our example. This is presumably much more likely in a complex, more involved Paragon component, possibly with subcomponents.
ButtonV2
is created in the main branch whenever it became necessary to make a breaking change.- Any new code that needs
ButtonV2
uses it with that name in the main branch. - A DEPR process is initiated for
Button
(v1), stating that it will be deprecated in Sumac, please don't use it for new work, and go upgrade your code toButtonV2
. - Sumac comes along, code still says
ButtonV2
and some warnings are printed in peoples' consoles aboutButton
stating to go upgrade toButtonV2
because the old implementation will be removed in Teak. - In Teak,
Button
either gets deleted or moved toButtonDeprecated
(or something) if people need more time.
ButtonV2
is renamed toButton
. Tooling could easily automate making this naming change for consumers (and I think Paragon already has this sort of thing)
So the only breaking change is in Teak, and involves 1) getting off of the old Button if you haven't in Sumac, and 2) renaming your button.
If the new code goes in as Button
, everyone on main has to immediately respond to the change (which may be nearly impossible if we're talking about shared dependencies via the shell), and anyone on Open edX Releases will have a breaking change in the very next release with no warning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it and we are in agreement! Thanks for filling in the specific Paragon/Frontend details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adamstankiewicz, very curious if you think the above sounds viable or completely insane. Or somewhere in between. 😂
- Clarifying the "Motivation" section to talk about how our MFEs are more like single-page apps than true micro-frontends. - Review feedback: correct package names around @edx/@openedx npm orgs and some punctuation fixes.
@davidjoy @arbrandes and all: I got a simple version of this working in Vite. I haven't yet analyzed the details, but you're welcome to play around with it! So far I only configured it to de-duplicate In the video, you'll see the shell provides the header and footer, and the profile app is loaded dynamically. vite-demo.movCode: https://github.com/open-craft/frontend-app-shell |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gotta go, here's some comments, will continue to review tomorrow. I'm loving it so far!
:widths: 25 75 | ||
|
||
* - OEP | ||
- :doc:`OEP-65 <oep-0065-arch-frontend-composability>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: This is the preferred way of titling the doc (oep-####-(type)-(name)
) but the actual title of this file doesn't have the arch
in it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we lose all the comments if I rename the doc... 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is actually possible. Maybe get it perfect on this PR, close the PR, and open a new one with the properly titled document - no discussion, just straight to merge?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, fixing it in a follow-up step sounds like a safer option.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super happy to see how much discussion is happening on this, and it's really coming together!
I left a couple more small comments, I'll give it another pass a bit later.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
In terms of Open edX MFEs, this means: | ||
|
||
1. MFEs can continue to be built independently. | ||
2. The Webpack build will include a manifest of which sub-modules the MFE provides at runtime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a note that moving to vite from webpack would require an ADR updating https://docs.openedx.org/projects/openedx-proposals/en/latest/best-practices/oep-0067-bp-tools-and-technology.html
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I learned a lot from this OEP! I have two notes that didn't fit in comments:
- I think the diagram is swell
- Are there any tradeoffs or downsides with this approach, ie, will module federation make anything suck a bit more from the current state?
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
|
||
Guests loading their own versions of shared dependencies degrades the performance and experience of end users. MFE authors should endeavor to use dependencies compatible with the version loaded by the shell. If we use a passthrough library of shared dependencies, this becomes easier. | ||
|
||
Converting the POC to a reference implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This feels like it should definitely be a living document. There are robust comments, questions, and suggestions in the comments for each of the line items that shouldn't be lost - and discussion should continue, past when this OEP is merged.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
- Added table of contents. - Editing for clarity throughout. - Linking out to Open edX repositories and dropping the organization prefix from their names.
Hi all - I've done another round, editing the document for clarity based on everyone's comments. Easiest way to review the changes may be to just go look at the diff of the last commit; there are little changes all over. Thank you all for the feedback! I think we're getting closer. I've taken the liberty of resolving comments on the PR that I think are now addressed in the document; please feel free to re-open or re-comment if you think something warrants more discussion. Technically we reached the end of the review period on April 29, but I expect @adamstankiewicz won't object to us taking a little extra time. Do we want to add another week or so? Perhaps until next Wed, 2024-05-08? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a "frontend baby" this looks good to me, pending some rst tweaks (blocking) and wording/structure suggestions (not blocking). Also, +1 to Sarina's points about avoiding speculation and writing in a style that makes sense 8 years from now.
Thanks for you great work on this!
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
- RST formatting changes. - Simplified language here and there. - Removing language around “today” and “current”, which will not age well for folks reading this OEP in the future. - Making Specification section easier to follow with better headers, and a more “oomphy”/active voice introduction. - Simplifying the shared dependency table, since most didn’t have “notes” anyway. - Adding links and references throughout. - Adding diagram “alt” text description and link to LucidChart source. - Removing the “Appendix” on how Module Federation works - the Specification and Reference Implementation sections cover it. - Removing secondary concerns around central data store and eventing - we seem to be in agreement that those are not concerns that we expect to have, so they feel superfluous for the OEP.
Another day, another set of revisions. Please see the PR description for changes. To my knowledge I've addressed everything that's been brought up on the PR. Let me know if you don't think that's true. 😛 |
After some discussion, we agreed that the ‘passthrough library’ idea is too experimental/unknown to add to the OEP. Instead, we’ve described the goals of reorganizing our code and suggested that we add a specific approach as an ADR on this OEP on it becomes more clear.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OEP-65's review period elapsed by an extra week. Thanks for the engaging feedback/questions from the reviewers and the updates/responses by the authors! OEP-65 will be merged and considered "Provisional" 🎉
- Extend the Webpack configuration in the MFEs by defining what modules each "guest" MFE exports. We suggest that the package.json `exports <https://nodejs.org/api/packages.html#subpath-exports>`_ field be used to codify this list of exports, and that Webpack pull it in from package.json to configure ``ModuleFederationPlugin``. The format appears to be the same. | ||
- Give "guest" MFEs a way of seeing their own config, since they'll be getting ``@edx/frontend-platform`` as a shared dependency from the shell, and won't be initializing it themselves. | ||
|
||
Secondary concerns include: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing I need to go sanity check in a POC is how react-router works with module federation and if there are somehow issues there.
In my early POC's with module federation from awhile ago, I recall routing being a concern; specifically with the sharing of a single instance of history
. For example, if a child MFE ends up with a different history
object than the parent/host MFE, any routing triggered from the child MFE may not work properly.
Hopefully the singleton approach with frontend-platform will mitigate that, though.
oeps/architectural-decisions/oep-0065-frontend-composability.rst
Outdated
Show resolved
Hide resolved
* - Arbiter | ||
- Adam Stankiewicz <astankiewicz@2u.com> | ||
* - Status | ||
- Under Review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Under Review | |
- Provisional |
* - Created | ||
- 2024-04-03 | ||
* - Review Period | ||
- April 15, 2024 - April 29, 2024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- April 15, 2024 - April 29, 2024 | |
- April 15, 2024 - May 10, 2024 |
@adamstankiewicz I didn't feel like we finished the conversation you started around Vite vs. webpack
I went so far as to build a POC that I think shows many advantages of Vite for this particular endeavour, but the last I heard regarding it was:
I was hoping for an answer either way before we consider this accepted, or if not then an acknowledgement that there is more investigation needed there. |
@bradenmacdonald, I feel like the POC you built demonstrates that Vite is definitely faster than Webpack, but as you put in your README comments, it doesn't actually demonstrate an approach akin to module federation that will allow for runtime module loading, shared dependencies, and independent deployment, which are the primary subjects of this OEP. I think we're all in agreement that there are faster build tools out there than Webpack... we just haven’t proven that any of them have a valid module federation implementation that gives us what we need. It appears there are two module federation implementations for Vite, https://github.com/module-federation/vite and https://github.com/originjs/vite-plugin-federation. It's unclear to me whether those allow for runtime definition of modules or whether the app needs to know all its remotes at build-time. The latter is, I think, insufficient for our use cases. I’d like to propose that we proceed with merging the OEP as “Provisional”, and would I'd argue that the premise of this OEP is still correct, even if we want to pause and do a little more research on whether the word "Webpack" can/should be replaced with the word "Vite" throughout. I think we should merge as is and update it as necessary. If we can find a compatible/reasonable replacement in Vite or some other, faster bundler, that will be really compelling and a great reason to amend the OEP. Related to this, though, I think there’s a separate question implicit in the Vite POC about whether maintaining independent deployments for MFEs is something we, as a community, still want to prioritize, or whether the majority of site operators would be better off if we had a simpler architectural model. Module federation is definitely making it more complex, not less in some regards. I think, long term, we can actually have the best of both architectures. This is a glimpse of a future vision, but by unifying frontend-plugin-framework with module federation, we should be able to allow operators to statically link in MFEs and components at build-time by default, like Braden’s POC, and then override those links with alternative components for customization/extension. These customizations could be loaded at build time, via iframes, or via module federation, all via the plugin framework’s configuration. So, er, anyway, definitely acknowledge there's more we can do here, but I don't think it's necessarily at odds with the OEP in its current form. |
This is to satisfy the build - after we merge the current PR openedx#575 we’ll be renaming the document to ‘oep-0065-arch-frontend-composability.rst’ as a separate PR.
Setting status as “Provisional” pending merge of the PR.
Thanks @davidjoy. That sounds good to me, and with that now stated explicitly I'm good with merging this 👍🏻 . BTW I offered to set up the Vite module federation plugins to demonstrate how they work, but didn't hear back if that would be useful. It sounds like it would be? |
@davidjoy 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
@bradenmacdonald I missed that offer! Yeah, that'd be great if you have a little time. I got as far as digging around in their code, but couldn't quite tell if they could do what we needed. |
This PR is a replacement for #410 based on an updated architectural approach. We expect to use module federation instead of adopting a framework like Piral.
@arbrandes and I agreed we should create this PR and close #410 so we can start with a bit of a clean slate after the lengthy conversation on that one. Some of the comments over there are definitely still applicable; if you feel a point got lost and hasn't been taken into consideration in this version of the OEP, feel free to comment again.
Running changelog based on PR feedback:
2024-05-03
2024-05-01
2024-04-22
2024-04-12
2024-04-10
2024-04-09
2024-04-04
Pull request #575 <https://github.com/openedx/open-edx-proposals/pull/575>
_2024-04-03