[WIP] fix: add script to install node_modules in mounted mfes #232

hinakhadim · 2024-10-31T11:44:40Z

WIP -> The jobs run on tutor dev only

Description

These code changes introduce improvements to the Tutor MFE (Micro-Frontend) plugin by adding automation and efficiency for managing and running tasks in environments where MFEs are mounted. When MFEs are mounted, it will run npm clean-install as part of the run tutor dev launch. The "mfe" init task npm-install.sh is taking care of installing the node_modules, as it is done in the LMS

Test Cases:

It will run the npm-install script when MFEs are implicitly and expicitly mounted using the Tutor format: tutor mounts add $(pwd)/frontend-app-* & tutor mounts add [mfe-name]:$(pwd)/frontend-app-*:/openedx/app.
The job should run for all mounted MFEs
The job will run only in tutor dev, not tutor local, as MFEs mount only in tutor dev environment.
If no node_modules are present, it should install node_modules.
If the node_modules directory exists but is empty or outdated, it should install node_modules.
If the package-lock.json file changes slightly, it should trigger reinstallation of node_modules.
If node_modules are installed and then reinstallation is attempted without changes to package-lock.json, it should not reinstall node_modules.

Closes #218

Special thanks to @Danyal-Faheem for collaborating with me on this and helping to reach a solution. 🙌

Danyal-Faheem · 2024-10-31T12:03:05Z

tutormfe/plugin.py

+def _run_jobs_in_mounted_mfes(config: Config) -> None:
+    mounts = get_typed(config, "MOUNTS", list, [])
+
+    pattern = re.compile(r'frontend-app-(\w+)')


We can use the REPO_PREFIX variable defined above here instead of a hardcoded string.

Danyal-Faheem · 2024-10-31T12:11:11Z

tutormfe/plugin.py

+@tutor_hooks.Actions.CONFIG_LOADED.add()
+def _run_jobs_in_mounted_mfes(config: Config) -> None:
+    mounts = get_typed(config, "MOUNTS", list, [])
+


We can return here if no mounts exist in the config.

DawoudSheraz · 2024-11-01T06:43:11Z

tutormfe/patches/dev-docker-compose-jobs-services

@@ -0,0 +1,11 @@
+{%- for app_name, app in iter_mfes() %}


why is this patch needed?

This patch is required to add the necessary jobs to run the init jobs inside the containers of the MFEs.

DawoudSheraz · 2024-11-01T06:44:26Z

tutormfe/plugin.py

+    pattern = re.compile(r'frontend-app-(\w+)')
+    mounted_mfes = [match.group(1) for mount in mounts if (match := pattern.search(mount))]
+
+    mfe_task_file = os.path.join(


mfe_npm_install could be a better name as this action is related only to npm install.

DawoudSheraz · 2024-11-01T06:46:46Z

tutormfe/templates/mfe/tasks/mfe/npm-install.sh

+  sha1sum "$PACKAGE_LOCK" | awk '{print $1}'
+}
+
+# Check if node_modules exists and fingerprint is valid


this comment can be broken down and mentioned against the appropriate if-elif-else block.

regisb · 2024-11-19T12:06:59Z

tutormfe/plugin.py

+
+
+@tutor_hooks.Actions.CONFIG_LOADED.add()
+def _run_jobs_in_mounted_mfes(config: Config) -> None:


I'm not a big fan of adding callbacks to the CONFIG_LOADED action. It means we have to manually parse MOUNTS and this is bound to fail in many edge cases. Also, we have to start using Context objects, which is VERY dirty.

Instead, I suggest the following:

@tutor_hooks.Filters.CLI_DO_INIT_TASKS.add() def _mfe_npm_install(tasks): for mfe in get_mfes(): if hooks.Filters.COMPOSE_MOUNTS.apply([], f"frontend-app-{mfe}"): tasks.append((mfe, "... contents of npm-install.sh..."))

What do you think?

At first glance I agree with @regisb, here. If there are no technical blockers to doing it this way, I think it would be less problematic than adding a -jobs Docker service that needs to be duplicated in both dev and local compose files.

We're already assuming frontend-app-* in REPO_PREFIX anyway, and using it in different mount-related places. No harm in re-using it.

Danyal-Faheem · 2025-05-13T11:46:38Z

tutormfe/templates/mfe/tasks/mfe/npm-install.sh

+
+# Store the new fingerprint after installation
+generate_fingerprint > "$FINGERPRINT_FILE"
+echo "✅ Dependencies installed and fingerprint updated."


nit: add extra line at end of file

Danyal-Faheem · 2025-05-13T11:46:47Z

tutormfe/patches/dev-docker-compose-jobs-services

+        - {{ mount }}
+        {%- endfor %}
+{%- endif %}
+{%- endfor %}


nit: add extra line at end of file

Danyal-Faheem · 2025-05-13T11:48:05Z

tutormfe/patches/dev-docker-compose-jobs-services

@@ -0,0 +1,11 @@
+{%- for app_name, app in iter_mfes() %}
+{%- set mounts = iter_mounts(MOUNTS, app_name)|list %}


We moved from iter_mounts to a cleaner MFEMountData object in #241. We should use the similar format here for consistency.

Danyal-Faheem · 2025-05-13T11:48:46Z

tutormfe/patches/local-docker-compose-jobs-services

@@ -0,0 +1,7 @@
+{%- for app_name, app in iter_mfes() %}


Why is this patch necessary? Don't we just need to run the init job in dev?

Danyal-Faheem · 2025-05-13T12:00:33Z

tutormfe/plugin.py

+        return None
+
+    pattern = re.compile(rf'{re.escape(REPO_PREFIX)}(\w+)')
+    mounted_mfes = [match.group(1) for mount in mounts if (match := pattern.search(mount))]


Looking back, we should be able to use MFEMountData here as well instead of using a regex. What do you think?

DawoudSheraz · 2025-06-25T07:56:02Z

tutormfe/patches/local-docker-compose-jobs-services

+# We only need to define these for local as well otherwise docker throws a not found error when we run the local init jobs
+# These jobs are required to run the npm install script when mounting MFEs
+{%- set mfe_data = MFEMountData(MOUNTS) %}
+
+{%- for app_name, app, mounts in mfe_data.mounted %}
+{{ app_name }}-job:
+    image: "{{ MFE_DOCKER_IMAGE_DEV_PREFIX }}-{{ app_name }}-dev:{{ MFE_VERSION }}"
+    volumes:
+        {%- for mount in mounts %}
+        - {{ mount }}
+        {%- endfor %}
+    environment:
+        - "NODE_ENV=production"
+{%- endfor %}


I dont understand why this is needed. It is using dev prefixes in local/production env, which in itself is not coherent. Why is local init job failing if this is not provided?

We add a job for every mounted MFE to the CLI_DO_INIT_TASKS filter. When we run the initialization task with the local context, aka tutor local do init, it will try to run those jobs that we added. However, we never defined a docker compose service to run that job in, therefore it fails.

We define this job structure here to let docker know where to run the job for each MFE. The job will still run in local mode but will exit due to the NODE_ENV environment variable being set to production.

The reason we added dev prefixes for local is just for consistency as we need to task to exit after discovering that NODE_ENV is set to production.

We add a job for every mounted MFE to the CLI_DO_INIT_TASKS filter. When we run the initialization task with the local context, aka tutor local do init, it will try to run those jobs that we added. However, we never defined a docker compose service to run that job in, therefore it fails.

Let's only do this for dev context. It is confusing to have this for local when it is not meant to be used.

Right, we tried to find a way to do that, however we are using the CONFIG_LOADED action to parse mounts and add our jobs. This action does not provide us any information if the job is being run with the local or dev context and therefore, we can't add it conditionally.

The only hook that provides this information is the APP_PUBLIC_HOSTS filter and we cannot use it as it doesn't provide us information regarding mounts. This is why we had no choice but to run the job in both local and dev containers and then exit if a production ENV is discovered.

@arbrandes what do you think about this?

@arbrandes Ping 🙂

Have you given @regisb's suggestion a try? I too feel that having this in the local file is troublesome, and doing it some other way (i.e., a DO_INIT hook that iterates over COMPOSE_MOUNTS) looks like it would not require adding a -jobs service.

arbrandes

Before moving forward, I'd like to understand if a less invasive alternative (@regisb's suggestion, specifically) was investigated.

arbrandes · 2025-09-19T17:38:17Z

tutormfe/patches/local-docker-compose-jobs-services

+# We only need to define these for local as well otherwise docker throws a not found error when we run the local init jobs
+# These jobs are required to run the npm install script when mounting MFEs
+{%- set mfe_data = MFEMountData(MOUNTS) %}
+
+{%- for app_name, app, mounts in mfe_data.mounted %}
+{{ app_name }}-job:
+    image: "{{ MFE_DOCKER_IMAGE_DEV_PREFIX }}-{{ app_name }}-dev:{{ MFE_VERSION }}"
+    volumes:
+        {%- for mount in mounts %}
+        - {{ mount }}
+        {%- endfor %}
+    environment:
+        - "NODE_ENV=production"
+{%- endfor %}


Have you given @regisb's suggestion a try? I too feel that having this in the local file is troublesome, and doing it some other way (i.e., a DO_INIT hook that iterates over COMPOSE_MOUNTS) looks like it would not require adding a -jobs service.

arbrandes · 2025-09-19T17:44:56Z

tutormfe/plugin.py

+
+
+@tutor_hooks.Actions.CONFIG_LOADED.add()
+def _run_jobs_in_mounted_mfes(config: Config) -> None:


At first glance I agree with @regisb, here. If there are no technical blockers to doing it this way, I think it would be less problematic than adding a -jobs Docker service that needs to be duplicated in both dev and local compose files.

We're already assuming frontend-app-* in REPO_PREFIX anyway, and using it in different mount-related places. No harm in re-using it.

hinakhadim requested review from Danyal-Faheem, DawoudSheraz, arbrandes and regisb October 31, 2024 11:47

hinakhadim assigned hinakhadim and Danyal-Faheem Oct 31, 2024

hinakhadim added the bug Something isn't working label Oct 31, 2024

hinakhadim marked this pull request as draft October 31, 2024 11:48

hinakhadim changed the title ~~fix: add script to install node_modules in mounted mfes~~ [WIP] fix: add script to install node_modules in mounted mfes Oct 31, 2024

Danyal-Faheem reviewed Oct 31, 2024

View reviewed changes

DawoudSheraz reviewed Nov 1, 2024

View reviewed changes

regisb reviewed Nov 19, 2024

View reviewed changes

DawoudSheraz assigned HammadYousaf01 and unassigned hinakhadim Feb 4, 2025

HammadYousaf01 moved this from In Progress to In review in Tutor project management May 6, 2025

Danyal-Faheem self-requested a review May 13, 2025 11:45

Danyal-Faheem suggested changes May 13, 2025

View reviewed changes

Danyal-Faheem reviewed May 13, 2025

View reviewed changes

hinakhadim and others added 4 commits June 17, 2025 19:25

fix: add script to install node_modules in mounted mfes

9438a66

fix: skip npm install in local

3a63d4a

fix: address comments

5fd1571

refactor: use MFEMountData instead of parsin mounts

8b819e6

Danyal-Faheem force-pushed the fix/mounted-mfes-npm-install branch from 49bb54f to 8b819e6 Compare June 18, 2025 14:15

chore: format files

f94c755

Danyal-Faheem requested a review from DawoudSheraz June 18, 2025 14:21

DawoudSheraz marked this pull request as ready for review June 24, 2025 09:08

DawoudSheraz reviewed Jun 25, 2025

View reviewed changes

ahmed-arb moved this from In review to Blocked in Tutor project management Jul 9, 2025

arbrandes requested changes Sep 19, 2025

View reviewed changes

DawoudSheraz unassigned Danyal-Faheem Sep 22, 2025



		@tutor_hooks.Actions.CONFIG_LOADED.add()
		def _run_jobs_in_mounted_mfes(config: Config) -> None:

		@@ -0,0 +1,11 @@
		{%- for app_name, app in iter_mfes() %}
		{%- set mounts = iter_mounts(MOUNTS, app_name)\|list %}

[WIP] fix: add script to install node_modules in mounted mfes #232

Are you sure you want to change the base?

[WIP] fix: add script to install node_modules in mounted mfes #232

Uh oh!

Conversation

hinakhadim commented Oct 31, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

WIP -> The jobs run on tutor dev only

Description

Test Cases:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arbrandes Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arbrandes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

arbrandes Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hinakhadim commented Oct 31, 2024 •

edited

Loading

arbrandes Sep 19, 2025 •

edited

Loading

arbrandes Sep 19, 2025 •

edited

Loading