-
Notifications
You must be signed in to change notification settings - Fork 32
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: Adding ADRs 0002, 0003 and a glossary to OEP-65 #626
base: master
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @davidjoy! What's next?Please work through the following steps to get your changes 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. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by 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. |
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 looks great! I left a couple comments to start a couple conversations about how to communicate the changes, but the changes themselves are awesome!
oeps/architectural-decisions/oep-0065/decisions/0002-frontend-app-migrations.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065/decisions/0002-frontend-app-migrations.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065/decisions/0002-frontend-app-migrations.rst
Outdated
Show resolved
Hide resolved
oeps/architectural-decisions/oep-0065/decisions/0002-frontend-app-migrations.rst
Outdated
Show resolved
Hide resolved
4f0e6c2
to
674581e
Compare
oeps/architectural-decisions/oep-0065/decisions/0002-frontend-app-migrations.rst
Outdated
Show resolved
Hide resolved
Also fixing a broken link in oep 65 and the title of ADR 0001, which doesn’t need to say “OEP-65” in it.
Subsequent commits will start using this glossary via :term: roles.
Also linking out to other documents more liberally.
…2 and 3 This also liberally uses the glossary in the ADR.
This commit updates the language of OEP-65 to match its three ADRs, and also makes use of the frontend glossary where possible. Many of the terms used in the glossary don’t actually come up in the OEP.
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 awesome, thank you! I'm on the cusp of approving it, but as per the review comments, I'd like to discuss a couple of the choices first.
@@ -245,8 +248,12 @@ Process | |||
|
|||
We need to ensure maintainers and developers know what dependency versions to use, and when they need to upgrade to stay consistent. Open edX release documentation should include information on which frontend dependency versions are compatible with the release, likely pinned to a major version (i.e., React 17.x, Paragon 22.x, etc.) | |||
|
|||
Further, we recommend that each Open edX release have a single supported major version of all shared dependencies, and that all MFEs be upgraded to it prior to release. |
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.
Excellent addition.
* Use the :term:`shell` to provide the header, footer, and runtime initialization code, amongst other things. | ||
* Organize their code into loosely-coupled top-level components, which are called :term:`application modules <Application Module>`. | ||
|
||
As we adopt ``frontend-base``, the libraries it replaces will undergo their own deprecation processes, which will need to coordinate with the migration of micro-frontends included in Open edX releases. After that deprecation, the micro-frontend architecture will cease to be supported. |
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 we adopt ``frontend-base``, the libraries it replaces will undergo their own deprecation processes, which will need to coordinate with the migration of micro-frontends included in Open edX releases. After that deprecation, the micro-frontend architecture will cease to be supported. | |
As we adopt ``frontend-base``, the libraries it replaces will undergo their own deprecation processes, which we'll need to coordinate with the migration of micro-frontends included in Open edX releases. After that deprecation, the micro-frontend architecture will cease to be supported. |
Decision | ||
******** | ||
|
||
Each of our ``frontend-app-*`` repositories will migrate from being an independent "micro-frontend application" to being a library of modules that can be loaded into a common :term:`Shell`, deployed as a :term:`Site`. These are called :term:`application module libraries <Application Module Library>`. We will document the migration process in detail. At a high level, this will involve the following changes. |
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 think I understand why "Application Module Library" (each former MFE will in theory be able to export multiple modules, making it a library), but I have to ask: would trimming it to "Application Module" also work? It would be easier to remember, pronounce, etc.
Also, if we make the new name too difficult (however accurate), people will just fall back to calling them MFEs. (Which people probably will 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.
I think I'm going to drop the word "Application" from it so that it's just 'module library'. I think we should consider, at some point, whether these repos get renamed to be prefixed with frontend-library-
or something; I'm not prepared to bite off a real suggestion there yet. :)
The ``fedx-scripts`` CLI tools from ``frontend-build`` will be replaced with the ``openedx`` CLI tools from ``frontend-base``. We'll discuss some of them in detail here, as they help illustrate what the library will be able to do: | ||
|
||
* ``dev`` will start a dev server, loading the repository's application modules into the shell in a site. | ||
* ``dev:module`` will start a dev server that provides the application modules via module federation. |
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.
Nice!
* ``serve`` will work with ``build`` or ``build:module`` to locally serve the production assets they generated. | ||
* ``pack`` will work with ``release`` to create a ``.tgz`` file suitable for installing in local git checkouts that depend on the library. (this is a development tool) | ||
|
||
The ``dev``, ``dev:module``, ``build``, and ``build:module`` CLI commands will rely on the existence of a :term:`Site Config` file (the replacement for .env/env.config files) which will not be checked into the repository. |
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.
Not necessarily a suggestion, but a request for clarification:
The ``dev``, ``dev:module``, ``build``, and ``build:module`` CLI commands will rely on the existence of a :term:`Site Config` file (the replacement for .env/env.config files) which will not be checked into the repository. | |
The ``dev``, ``dev:module``, ``build``, and ``build:module`` CLI commands will rely on the existence of a :term:`Site Config` file (the replacement for .env/env.config files) which will not be checked into the module repository. |
The config file will be checked into a repository (the project's, I suppose), just not the module's... right?
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.
Good clarification, fixing.
|
||
Our definition of :term:`module` aligns with the `industry standard definition <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Modules>`_. It is also used in the context of `module federation <https://module-federation.io>`_. It's a self-contained part of the frontend that represents a specific part of the :term:`Site`, and can be loaded in a variety of ways. We have several sub-types of module: | ||
|
||
* An *application module* represents a well-bounded sub-area of the Open edX frontend at a particular route path. This might be "courseware", "the login page", or "account settings". There are a number of application modules that are *required* for a functioning Open edX frontend Site. |
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 per a previous review comment, I feel "application module" can double as the name for the whole thing (sans library at the end).
Maybe another way to think about it is that different modules should live in different repos, lest we fall into the mono-repo issues we've discussed in the past.
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 talked about this offline, but for posterity - there are two related rationales for keeping modules (particularly application modules, which are generally pretty big) in the same repository together. These are:
- If they share code with other modules in the library. If we split them up and have to create a secondary library to share code between them, we probably haven't made our lives better.
- If they're part of the same conceptual 'domain' of the site. This was the original idea behind the MFE boundaries we have today, though we didn't execute it consistently. Some are too big, some are too small. It seems very reasonable to me that the learning-related stuff could be in "frontend-app-learning", and the user account/authentication related stuff would be in something like "frontend-app-user", which doesn't exist.
|
||
Addressing our *lack* of dependency version consistency is one of the primary drivers of OEP-65. | ||
|
||
The shell will support specific versions of shared dependencies (such as React, Paragon, or React Router). All applications loaded into the shell's Site will be expected to use (or at least be compatible) with that version. We intend to create lock-step version consistency of shared dependencies across all applications in the platform. We envision each Open edX release supporting a particular major version of each shared dependency. |
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.
+100
5. The Shell initialization code imports the :term:`Site Config` (i.e., ``site.config.build.tsx``) file from the project. | ||
6. The :term:`Site Config` file imports any :term:`Application Modules <Application Module>` from libraries it depends on (defined as ``dependencies`` in package.json), along with any other Modules from the ``src`` sub-folder. | ||
|
||
Module Projects |
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.
While Site Projects are clear to me off the bat, I'm wondering why we also need module projects. I suspect this is a consequence of the different ways modules can be built, but I was under the impression different package.json build targets in the modules themselves would be enough.
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'm putting a clarifying paragraphs about why the two exist at the top of their sections, which hopefully clears it up a bit.
@davidjoy @arbrandes -- what's left on this one? |
Just awaiting final review. Didn't want to bug Axim during the on-site week. |
@arbrandes - any timeline for a final review? |
This PR includes a bunch of stuff: