[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?

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?

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?

fix: add script to install node_modules in mounted mfes

7b1f7e9

hinakhadim requested review from regisb, arbrandes, DawoudSheraz and Danyal-Faheem 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

fix: skip npm install in local

49bb54f

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

hinakhadim commented Oct 31, 2024 •

edited

Loading

Uh oh!

Danyal-Faheem Oct 31, 2024

Uh oh!

Danyal-Faheem Oct 31, 2024

Uh oh!

DawoudSheraz Nov 1, 2024

Uh oh!

DawoudSheraz Nov 1, 2024

Uh oh!

DawoudSheraz Nov 1, 2024

Uh oh!

DawoudSheraz Nov 1, 2024

Uh oh!

regisb Nov 19, 2024

Uh oh!

Danyal-Faheem May 13, 2025

Uh oh!

Danyal-Faheem May 13, 2025

Uh oh!

Danyal-Faheem May 13, 2025

Uh oh!

Danyal-Faheem May 13, 2025

Uh oh!

Danyal-Faheem May 13, 2025

Uh oh!

Uh oh!



		@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!

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!

Uh oh!

hinakhadim commented Oct 31, 2024 •

edited

Loading