feat: add dependency updater command#222
Conversation
d82fac4 to
99edb00
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds a new MageForge CLI workflow to update theme-owned Node.js dependencies (including Hyvä/Tailwind theme layouts), backed by a new DependencyUpdater service and enhancements to NodePackageManager, plus command reference documentation updates.
Changes:
- Added
mageforge:dependencies:updateCLI command with dry-run and--latest(force/pin) update modes. - Introduced
DependencyUpdaterservice to locate theme-ownedpackage.jsonlocations and orchestrate safe vs latest updates. - Extended
NodePackageManagerwith outdated parsing and dependency-type-aware install flags; added/updated unit tests and docs.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Unit/Service/NodePackageManagerTest.php | Adds coverage for parsing npm outdated JSON output and new update/install helper methods. |
| tests/Unit/Service/DependencyUpdaterTest.php | Adds unit tests for the new updater service across dry-run/latest/vendor/no-package scenarios. |
| tests/Unit/Console/Command/Dependencies/UpdateCommandTest.php | Adds unit tests for the new CLI command’s argument/option handling and summary output. |
| src/Service/NodePackageManager.php | Adds normalized npm outdated parsing plus npm update and dependency-type-aware installs. |
| src/Service/DependencyUpdater.php | New service coordinating dependency updates across theme package directories with safety checks. |
| src/etc/di.xml | Registers the new CLI command with Magento’s command list. |
| src/Console/Command/Dependencies/UpdateCommand.php | Implements the new mageforge:dependencies:update command, including interactive selection and summaries. |
| docs/commands_reference.md | Documents the new command, options, behavior, and adds it to command listings. |
|
@Morgy93 is lumaand magento base supported ?package.json in magento root btw. Skipped if no luma is in use 🤔 |
Yes, since luma has no own dependencies, there is a fallback to the root package.json. |
5385782 to
291d868
Compare
|
i resolved (rebased) the merge-conflicts |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/Service/DependencyUpdater.php:91
- The early
isVendorTheme()check prevents Magento core themes (e.g.Magento/luma) from reaching the Magento-root fallback when they are installed under<magento_root>/vendor/...(which is the default for Composer-installed Magento themes). That contradicts the intended behaviour described in this service/docstring. Consider resolving package directories / standard-theme fallback first, and only applying the vendor-skip when the theme actually has its ownpackage.jsonto update.
$themePath = rtrim($themePath, '/');
if ($this->isVendorTheme($themePath)) {
$io->warning(sprintf(
"Theme '%s' is installed in the vendor directory and is managed by Composer. Skipping.",
$themeCode,
));
return DependencyUpdateResult::Skipped;
}
$packageDirectories = $this->getPackageDirectories($themePath);
if (empty($packageDirectories)) {
return $this->updateMagentoRootDependencies($themeCode, $themePath, $io, $isVerbose, $latest, $dryRun);
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
tests/Unit/Service/DependencyUpdaterTest.php:240
- Mock expectation for NodePackageManager::updatePackages() only matches the path argument, but the method is called with ($path, $io, $isVerbose). This will fail the test due to argument mismatch.
$this->nodePackageManager
->expects($this->once())
->method('updatePackages')
->with('/theme/web/tailwind')
->willReturn(true);
| function (string $path, string $type, array $packages) use (&$installedGroups): bool { | ||
| $installedGroups[$type] = $packages; | ||
| $this->assertSame('/theme/web/tailwind', $path); | ||
| return true; | ||
| }, |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
tests/Unit/Service/DependencyUpdaterTest.php:240
- Mock expectation for NodePackageManager::updatePackages() uses the old one-argument signature. The implementation now calls updatePackages($path, $io, $isVerbose), so this test will fail due to argument mismatch.
$this->nodePackageManager
->expects($this->once())
->method('updatePackages')
->with('/theme/web/tailwind')
->willReturn(true);
Fixes #204
This pull request introduces a new CLI command for updating Node.js dependencies of Magento themes and improves documentation and workflow coverage for this feature. The main changes include the implementation of the
mageforge:dependencies:updatecommand, comprehensive documentation for its usage and behavior, and integration into the compatibility workflow.New CLI Command for Dependency Updates:
mageforge:dependencies:updatecommand (src/Console/Command/Dependencies/UpdateCommand.php) to update Node.js dependencies for one or more themes, supporting interactive and non-interactive usage, wildcards, dry-run, and updating to latest versions.DependencyUpdateResultenum to standardize the result of dependency update operations (src/Service/DependencyUpdateResult.php).Documentation Updates:
docs/commands_reference.mdto add the new command to the overview table, provide a detailed reference section, and include it in the quick list of commands. [1] [2] [3]Workflow Enhancements:
.github/workflows/magento-compatibility.ymlto test the new command and its alias, ensuring coverage in CI. [1] [2]