[WIP] Build process for frontendbase apps alternative explicit version - #283
[WIP] Build process for frontendbase apps alternative explicit version#283holaontiveros wants to merge 10 commits into
Conversation
arbrandes
left a comment
There was a problem hiding this comment.
Thanks, Javier!
In this first pass, I made several inline comments, but as I write this some of them are already obsolete lol. Anyway, the gist of it comes down to:
- We should not depend on an arbitrary prefix to distinguish frontend apps from regular MFEs. They should be "their own thing" as much as possible.
- We need a separate section/loop of the Dockerfile from the one that builds regular MFEs, because the requirements and expectations, though similar, are not quite the same.
- tutor-mfe needs its own minimal version of frontend-template-site. As in, it should be in this repository, not in a separate one. We'd just call it
frontend-site, or something like that. This is because we're going to have to let Tutor customize certain files via patches: notably,package.jsonandindex.html.
And now the important bit:
- NPM workspaces: I believe we can reconcile the explicit build being proposed here (which I prefer) with some of the advantages of the
npm prepareapproach you suggest in #282 with NPM workspaces - but even better. The basic plan is the following:
package.json differences from frontend-template-site
Add a workspaces field and add the build:deps script:
{
"workspaces": [
"packages/*"
],
"scripts": {
"build:deps": "npm run build --workspaces --if-present",
}
}Dependencies like @openedx/frontend-app-learner-dashboard remain listed under dependencies with their registry versions. When a matching package exists under packages/, npm resolves the dependency via the workspace checkout instead of fetching from the registry.
Local checkouts
Apps that need building (and only apps that need building) are checked out into frontend-site/packages/. This can be done in the aforementioned separate loop in the Dockerfile, all in a single layer.
When no packages are checked out, npm install falls back to the registry as usual — the same package.json works both ways.
Build Workflow
This is the single install/build step in the Dockerfile after everything is checked out:
npm install # hoists and deduplicates all dependencies, including devDependencies like tsc and tsc-alias
npm run build:deps # builds the workspace packages (and just those)
npm run build # build the sitenpm run build --workspaces runs each package's build script in topological order. --if-present skips packages that don't have one.
Advantages Over prepare in Each Package
- Shared build tools:
tsc,tsc-alias, and other build dependencies are installed once at the root, not independently in each package. Faster installs, less disk usage. - Explicit build step:
build:depsis a deliberate action, not a side effect ofnpm install. Easier to reason about and debug. - No devDependency churn: With
prepareon git dependencies, npm installs each package's devDependencies, runs the build, then prunes them. Workspaces skip this install-build-prune cycle entirely. - Graceful fallback: Without
packages/checkouts, the project installs normally from the registry.
| uri strip_prefix /{{ app_name }} | ||
| {%- if is_frontend_app_enabled(app_name) %} | ||
| # {{ app_name }} - using frontend-apps approach | ||
| root * /openedx/dist/template-site |
There was a problem hiding this comment.
How about calling it frontend-site?
| root * /openedx/dist/template-site | |
| root * /openedx/dist/frontend-site |
There was a problem hiding this comment.
Agree, this is currently based on the naming for the especific repo, so given that it's called "template-site" it ends up with that name, so it's a matter of changing the config
| MFE_ATTRS_TYPE = t.Dict[ | ||
| t.Literal["repository", "port", "version"], t.Union["str", int] | ||
| ] | ||
| FRONTEND_TEMPLATE_SITE_ATTRS_TYPE = t.Dict[ |
There was a problem hiding this comment.
Unless I misunderstood the purpose, this is about individual apps, not the whole site, right?
| FRONTEND_TEMPLATE_SITE_ATTRS_TYPE = t.Dict[ | |
| FRONTEND_APP_ATTRS_TYPE = t.Dict[ |
| # "repository": "https://github.com/WGU-Open-edX/frontend-template-site.git", | ||
| # "version": "initial", | ||
| # "port": 8080, | ||
| # } |
There was a problem hiding this comment.
Can we remove this commented out bit?
There was a problem hiding this comment.
I had it to remember to add the template site as a default app, BUT if we are going with base files to be patched then yeah I'll just kill this part
| # List will need | ||
| ## Apps that are only frontend-apps | ||
| ## Apps that are only MFEs | ||
| ## Apps with unique ones (all old mfes + instruct) | ||
| ## 1 and 2 with 1 having something like a different identifier |
There was a problem hiding this comment.
Without reading the code, this comment is hard to understand. What are "apps with unique ones"? What are 1 and 2?
There was a problem hiding this comment.
Agree, It was for my own read when I was iteraring through different approachces, I'll clean it as soon as we get to the final draft
|
|
||
|
|
||
| @tutor_hooks.lru_cache | ||
| def get_frontend_apps(apps_to_build: bool = False) -> dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]: |
There was a problem hiding this comment.
apps_to_build sounds like a list, but apparently it's a boolean. Let's call it something like only_apps_to_build, then?
| def get_frontend_apps(apps_to_build: bool = False) -> dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]: | |
| def get_frontend_apps(only_apps_to_build: bool = False) -> dict[str, FRONTEND_TEMPLATE_SITE_ATTRS_TYPE]: |
| } | ||
| handle @mfe_{{ app_name }} { | ||
| uri strip_prefix /{{ app_name }} | ||
| {%- if is_frontend_app_enabled(app_name) %} |
There was a problem hiding this comment.
Shorter and clearer, I think:
| {%- if is_frontend_app_enabled(app_name) %} | |
| {%- if is_frontend_app(path) %} |
| {{ patch("mfe-dockerfile-base") }} | ||
|
|
||
| {% for app_name, app in iter_mfes() %} | ||
| {% for app_name, app in iter_all_apps() %} |
There was a problem hiding this comment.
For clarity, as suggested elsewhere:
| {% for app_name, app in iter_all_apps() %} | |
| {% for app_name, app in iter_mfes_and_apps_to_build() %} |
|
|
||
| # While we figure out how translations will be managed in template site | ||
| # if it's either template-site or starts with frontend-app, we skip pulling translations | ||
| {% if app_name != "template-site" and not app_name.startswith("frontend-app") %} |
There was a problem hiding this comment.
I'm not a fan of distinguishing between MFE and frontend app just based on a string prefix. I think we should own up to the difference and have a separate loop just for frontend apps. For example, I suspect apps don't need the pre- and post-npm-install patches.
Also, I don't think frontend apps will be subject to the same MFE_COMMON_VERSION (we're not tagging frontend-base apps with release/verawood.1). This is another reason to have a separate list just for them.
There was a problem hiding this comment.
We can definitely separate them, I just didn't want to start with something separated without having everything rolling and being able to know how we can "optimize it"
I mean so the new separate loop does only the things we know we want it to do
| {{ patch("mfe-dockerfile-pre-npm-build") }} | ||
| {{ patch("mfe-dockerfile-pre-npm-build-{}".format(app_name)) }} | ||
|
|
||
| {% if is_frontend_app_to_build(app_name) %} |
There was a problem hiding this comment.
is_frontend_app() should be sufficient because this list only contains things to build already, but as noted in the above comment, I actually think we should have a separate loop just for frontend apps and avoid the check altogether.
| ######## {{ app_name }} (production) | ||
| FROM {{ app_name }}-common AS {{ app_name }}-prod | ||
|
|
||
| {% if app_name == "template-site" %} |
There was a problem hiding this comment.
The frontend site should not be an item on a configurable list. It should be a hard-coded part of the Dockerfile.
There was a problem hiding this comment.
So we'll have package.json, and a site.config.build.tsx tempaltes that we'll use as a base always and patch on top of it?
|
Forgot an important bit: I don't know if we'll have to build the list of dependencies in the frontend-site package.json dynamically from the plugin data, or if we can manage with separate |
|
Closing in favor of #284. |
Basic details
This is the first step for the build process of the frontend apps
Assumptions
This use multi stage builds if there's frontend apps that have the repository attribute configured which means that if you manually declare a repository for those apps the process will try to build it, pack it and install it.
** This may be network / RAM intensive ** so in case you run into something like:
disable a couple MFEs or reduce the amount of parallelism in docker build config (details about this can be found at the readme look for parallelism in the dev section)
Also for now, the aggregator repo needs to have the same public path as the app_name for the app in this case and for now that's template-site which means
PUBLIC_PATH=/template-site/because that will allow all the assets to be served properly.How to add a frontendapp
In order to mark something as a frontendbase app you need a plugin that looks like:
and if you need some of those to be manually build and installed in the template site:
notice that the
portis there but it's just to match the contract for the DockerfileAny app that it's on this list will be considered a fronend-app so when Caddy shows the content for it, it will show the one on template-site instead of the normal app.