From 0780ac217cb04ed5376627d0ddc9c39404e6338c Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Thu, 12 Sep 2024 16:23:40 +0100 Subject: [PATCH 01/28] Added inital work on npm commands --- modules/system/console/asset/AssetInstall.php | 29 +++++++- modules/system/console/asset/NpmCommand.php | 67 +++++++++++++++++++ modules/system/console/asset/NpmUpdate.php | 46 ------------- .../exceptions/PackageNotFoundException.php | 8 +++ .../PackageNotRegisteredException.php | 8 +++ .../system/console/asset/mix/MixInstall.php | 2 +- .../system/console/asset/npm/NpmInstall.php | 48 +++++++++++++ .../system/console/asset/{ => npm}/NpmRun.php | 50 ++------------ .../system/console/asset/npm/NpmUpdate.php | 47 +++++++++++++ .../system/console/asset/vite/ViteInstall.php | 2 +- 10 files changed, 213 insertions(+), 94 deletions(-) create mode 100644 modules/system/console/asset/NpmCommand.php delete mode 100644 modules/system/console/asset/NpmUpdate.php create mode 100644 modules/system/console/asset/exceptions/PackageNotFoundException.php create mode 100644 modules/system/console/asset/exceptions/PackageNotRegisteredException.php create mode 100644 modules/system/console/asset/npm/NpmInstall.php rename modules/system/console/asset/{ => npm}/NpmRun.php (52%) create mode 100644 modules/system/console/asset/npm/NpmUpdate.php diff --git a/modules/system/console/asset/AssetInstall.php b/modules/system/console/asset/AssetInstall.php index 10f5c78e95..6ecb911c33 100644 --- a/modules/system/console/asset/AssetInstall.php +++ b/modules/system/console/asset/AssetInstall.php @@ -8,6 +8,7 @@ use System\Classes\Asset\PackageJson; use System\Classes\Asset\PackageManager; use System\Classes\PluginManager; +use System\Console\Asset\Exceptions\PackageNotFoundException; use Winter\Storm\Console\Command; use Winter\Storm\Support\Facades\Config; use Winter\Storm\Support\Facades\File; @@ -194,14 +195,36 @@ protected function validateRequirePackagesPresent(PackageJson $packageJson): Pac protected function processPackages(array $registeredPackages, PackageJson $packageJson): PackageJson { - // Process each package + // Check if the user requested a specific package for install + $requestedPackage = strtolower($this->argument('assetPackage')); + $foundRequestedPackage = !$requestedPackage; + + if (!$foundRequestedPackage) { + foreach ($registeredPackages as $name => $package) { + if ($requestedPackage === $name) { + $foundRequestedPackage = true; + } + } + + // We did not find the package, exit + if (!$foundRequestedPackage) { + throw new PackageNotFoundException(sprintf( + 'The requested package `%s` could not be found. Try %s:config or check if the package is ignored.', + $this->argument('assetPackage'), + $this->assetType + )); + } + } + + // Process each found package foreach ($registeredPackages as $name => $package) { // Normalize package path across OS types $packagePath = Str::replace(DIRECTORY_SEPARATOR, '/', $package['path']); // Add the package path to the instance's package.json->workspaces->packages property if not present if (!$packageJson->hasWorkspace($packagePath) && !$packageJson->hasIgnoredPackage($packagePath)) { if ( - $this->confirm( + $requestedPackage === $name + || $this->confirm( sprintf( "Detected %s (%s), should it be added to your package.json?", $name, @@ -242,7 +265,7 @@ protected function processPackages(array $registeredPackages, PackageJson $packa */ protected function installPackageDeps(): int { - $command = $this->argument('npmArgs') ?? []; + $command = []; array_unshift($command, 'npm', $this->npmCommand); $process = new Process($command, base_path(), null, null, null); diff --git a/modules/system/console/asset/NpmCommand.php b/modules/system/console/asset/NpmCommand.php new file mode 100644 index 0000000000..d3e57b589e --- /dev/null +++ b/modules/system/console/asset/NpmCommand.php @@ -0,0 +1,67 @@ +fireCallbacks(); + + $name = $this->argument('package'); + + if (!$name) { + return null; + } + + if (!$compilableAssets->hasPackage($name, true)) { + throw new PackageNotRegisteredException(sprintf('Package "%s" is not a registered package.', $name)); + } + + $package = $compilableAssets->getPackage($name, true)[0] ?? []; + + // Assume that packages with matching names have matching package.json files + $packageJson = new PackageJson($package['package'] ?? null); + + return [$package, $packageJson]; + } + + protected function npmRun(array $command, string $path): int + { + $process = new Process( + $command, + base_path($path), + ['NODE_ENV' => $this->getNodeEnv()], + null, + null + ); + + try { + $process->setTty(true); + } catch (\Throwable $e) { + // This will fail on unsupported systems + } + + return $process->run(function ($status, $stdout) { + if (!$this->option('silent')) { + $this->getOutput()->write($stdout); + } + }); + } + + protected function getNodeEnv(): string + { + if (!$this->hasOption('production')) { + return 'development'; + } + + return $this->option('production', false) ? 'production' : 'development'; + } +} diff --git a/modules/system/console/asset/NpmUpdate.php b/modules/system/console/asset/NpmUpdate.php deleted file mode 100644 index 66665bca1a..0000000000 --- a/modules/system/console/asset/NpmUpdate.php +++ /dev/null @@ -1,46 +0,0 @@ - 'update', - 'completed' => 'updated', - ]; - - /** - * @inheritDoc - */ - protected string $npmCommand = 'update'; - - /** - * @inheritDoc - */ - public $replaces = [ - 'mix:update' - ]; -} diff --git a/modules/system/console/asset/exceptions/PackageNotFoundException.php b/modules/system/console/asset/exceptions/PackageNotFoundException.php new file mode 100644 index 0000000000..62b284d346 --- /dev/null +++ b/modules/system/console/asset/exceptions/PackageNotFoundException.php @@ -0,0 +1,8 @@ +getPackage(); + + $command = ($this->argument('npmArgs')) ?? []; + + if (!$command) { + return 0; + } + + array_unshift($command, 'npm', 'install'); + + return $this->npmRun($command, $package['path'] ?? ''); + } +} diff --git a/modules/system/console/asset/NpmRun.php b/modules/system/console/asset/npm/NpmRun.php similarity index 52% rename from modules/system/console/asset/NpmRun.php rename to modules/system/console/asset/npm/NpmRun.php index 8297cd41cd..d3fa1441fe 100644 --- a/modules/system/console/asset/NpmRun.php +++ b/modules/system/console/asset/npm/NpmRun.php @@ -1,13 +1,10 @@ fireCallbacks(); + [$package, $packageJson] = $this->getPackage(); - $name = $this->argument('package'); $script = $this->argument('script'); - if (!$compilableAssets->hasPackage($name, true)) { - $this->error( - sprintf('Package "%s" is not a registered package.', $name) - ); - return 1; - } - - $package = $compilableAssets->getPackage($name, true)[0] ?? []; - - // Assume that packages with matching names have matching package.json files - $packageJson = new PackageJson($package['package'] ?? null); - if (!$packageJson->hasScript($script)) { $this->error( - sprintf('Script "%s" is not defined in package "%s".', $script, $name) + sprintf('Script "%s" is not defined in package "%s".', $script, $this->argument('package')) ); return 1; } - $this->info(sprintf('Running script "%s" in package "%s"', $script, $name)); + $this->info(sprintf('Running script "%s" in package "%s"', $script, $this->argument('package'))); $command = ($this->argument('additionalArgs')) ?? []; if (count($command)) { @@ -75,25 +58,6 @@ public function handle(): int array_unshift($command, 'npm', 'run', $script); } - - $process = new Process( - $command, - base_path($package['path']), - ['NODE_ENV' => $this->option('production', false) ? 'production' : 'development'], - null, - null - ); - - try { - $process->setTty(true); - } catch (\Throwable $e) { - // This will fail on unsupported systems - } - - return $process->run(function ($status, $stdout) { - if (!$this->option('silent')) { - $this->getOutput()->write($stdout); - } - }); + return $this->npmRun($command, $package['path']); } } diff --git a/modules/system/console/asset/npm/NpmUpdate.php b/modules/system/console/asset/npm/NpmUpdate.php new file mode 100644 index 0000000000..134e563426 --- /dev/null +++ b/modules/system/console/asset/npm/NpmUpdate.php @@ -0,0 +1,47 @@ +getPackage(); + + $command = ($this->argument('npmArgs')) ?? []; + if (count($command)) { + array_unshift($command, 'npm', 'update', '--'); + } else { + array_unshift($command, 'npm', 'update'); + } + + return $this->npmRun($command, $package['path'] ?? ''); + } +} diff --git a/modules/system/console/asset/vite/ViteInstall.php b/modules/system/console/asset/vite/ViteInstall.php index 0e0c4ea8bf..683d085622 100644 --- a/modules/system/console/asset/vite/ViteInstall.php +++ b/modules/system/console/asset/vite/ViteInstall.php @@ -15,7 +15,7 @@ class ViteInstall extends AssetInstall * @var string The name and signature of this command. */ protected $signature = 'vite:install - {npmArgs?* : Arguments to pass through to the "npm" binary} + {assetPackage? : The asset package name to install} {--npm= : Defines a custom path to the "npm" binary} {--p|package=* : Defines one or more packages to install}'; From 2c6bea7adb8ed8858d2c3f80887bdc3629bcba38 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Thu, 12 Sep 2024 16:42:05 +0100 Subject: [PATCH 02/28] Added fix for root issued commands --- modules/system/console/asset/npm/NpmInstall.php | 16 +++++++++++----- modules/system/console/asset/npm/NpmUpdate.php | 10 ++++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/modules/system/console/asset/npm/NpmInstall.php b/modules/system/console/asset/npm/NpmInstall.php index 9848a48a76..aff7d90197 100644 --- a/modules/system/console/asset/npm/NpmInstall.php +++ b/modules/system/console/asset/npm/NpmInstall.php @@ -2,6 +2,7 @@ namespace System\console\asset\npm; +use System\Console\Asset\Exceptions\PackageNotRegisteredException; use System\Console\Asset\NpmCommand; class NpmInstall extends NpmCommand @@ -22,7 +23,8 @@ class NpmInstall extends NpmCommand protected $signature = 'npm:install {package? : The package name to add configuration for} {npmArgs?* : Arguments to pass through to the "npm" binary} - {--npm= : Defines a custom path to the "npm" binary}'; + {--npm= : Defines a custom path to the "npm" binary} + {--d|dev : Install packages in devDependencies}'; /** * @inheritDoc @@ -33,16 +35,20 @@ class NpmInstall extends NpmCommand public function handle(): int { - [$package, $packageJson] = $this->getPackage(); - $command = ($this->argument('npmArgs')) ?? []; - if (!$command) { - return 0; + try { + [$package, $packageJson] = $this->getPackage(); + } catch (PackageNotRegisteredException $e) { + array_unshift($command, $this->argument('package')); } array_unshift($command, 'npm', 'install'); + if ($this->option('dev')) { + $command[] = '--save-dev'; + } + return $this->npmRun($command, $package['path'] ?? ''); } } diff --git a/modules/system/console/asset/npm/NpmUpdate.php b/modules/system/console/asset/npm/NpmUpdate.php index 134e563426..74b7a64af0 100644 --- a/modules/system/console/asset/npm/NpmUpdate.php +++ b/modules/system/console/asset/npm/NpmUpdate.php @@ -2,6 +2,7 @@ namespace System\Console\Asset\Npm; +use System\Console\Asset\Exceptions\PackageNotRegisteredException; use System\Console\Asset\NpmCommand; class NpmUpdate extends NpmCommand @@ -33,9 +34,14 @@ class NpmUpdate extends NpmCommand public function handle(): int { - [$package, $packageJson] = $this->getPackage(); - $command = ($this->argument('npmArgs')) ?? []; + + try { + [$package, $packageJson] = $this->getPackage(); + } catch (PackageNotRegisteredException $e) { + array_unshift($command, $this->argument('package')); + } + if (count($command)) { array_unshift($command, 'npm', 'update', '--'); } else { From 05eaed41a587e3142c8429878e7891cb36ef6368 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Thu, 12 Sep 2024 16:47:18 +0100 Subject: [PATCH 03/28] Added npm:install command and moved scripts --- modules/system/ServiceProvider.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/system/ServiceProvider.php b/modules/system/ServiceProvider.php index 7867a54f24..c4f925b8fb 100644 --- a/modules/system/ServiceProvider.php +++ b/modules/system/ServiceProvider.php @@ -332,8 +332,9 @@ protected function registerConsole() $this->registerConsoleCommand('vite.list', Console\Asset\Vite\ViteList::class); $this->registerConsoleCommand('vite.watch', Console\Asset\Vite\ViteWatch::class); - $this->registerConsoleCommand('npm.run', Console\Asset\NpmRun::class); - $this->registerConsoleCommand('npm.update', Console\Asset\NpmUpdate::class); + $this->registerConsoleCommand('npm.run', Console\Asset\Npm\NpmRun::class); + $this->registerConsoleCommand('npm.install', Console\Asset\Npm\NpmInstall::class); + $this->registerConsoleCommand('npm.update', Console\Asset\Npm\NpmUpdate::class); } /* From cd62ba59cd98ac2f3e4439f356860e215508ebea Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Sun, 15 Sep 2024 19:30:12 +0100 Subject: [PATCH 04/28] Added better error reporting and moved tests --- .../system/classes/asset/PackageManager.php | 32 ++- modules/system/console/asset/AssetInstall.php | 183 +++++++++++------- .../exceptions/PackageIgnoredException.php | 8 + .../PackageNotConfiguredException.php | 8 + .../asset/{ => mix}/MixCompileTest.php | 4 +- .../console/asset/{ => mix}/MixCreateTest.php | 2 +- .../asset/{ => vite}/ViteCompileTest.php | 2 +- .../asset/{ => vite}/ViteCreateTest.php | 2 +- 8 files changed, 159 insertions(+), 82 deletions(-) create mode 100644 modules/system/console/asset/exceptions/PackageIgnoredException.php create mode 100644 modules/system/console/asset/exceptions/PackageNotConfiguredException.php rename modules/system/tests/console/asset/{ => mix}/MixCompileTest.php (99%) rename modules/system/tests/console/asset/{ => mix}/MixCreateTest.php (99%) rename modules/system/tests/console/asset/{ => vite}/ViteCompileTest.php (99%) rename modules/system/tests/console/asset/{ => vite}/ViteCreateTest.php (99%) diff --git a/modules/system/classes/asset/PackageManager.php b/modules/system/classes/asset/PackageManager.php index edb04c8571..829c03b052 100644 --- a/modules/system/classes/asset/PackageManager.php +++ b/modules/system/classes/asset/PackageManager.php @@ -24,6 +24,10 @@ class PackageManager { use \Winter\Storm\Support\Traits\Singleton; + public const TYPE_THEME = 'theme'; + public const TYPE_MODULE = 'module'; + public const TYPE_PLUGIN = 'plugin'; + /** * The filename that stores the package definition. */ @@ -239,10 +243,10 @@ public function hasPackage(string $name, bool $includeIgnored = false): bool public function getPackage(string $name, bool $includeIgnored = false): array { $results = []; - foreach ($this->packages ?? [] as $packages) { + foreach ($this->packages ?? [] as $type => $packages) { foreach ($packages as $packageName => $config) { if (($name === $packageName) && (!$config['ignored'] || $includeIgnored)) { - $results[] = $config; + $results[] = $config + ['type' => $type]; } } } @@ -330,6 +334,30 @@ public function registerPackage(string $name, string $path, string $type = 'mix' ]; } + public function getPackageTypeFromName(string $package): ?string + { + // Check if package could be a module + if (Str::startsWith($package, 'module-') && !in_array($package, ['system', 'backend', 'cms'])) { + return static::TYPE_MODULE; + } + + // Check if package could be a theme + if ( + in_array('Cms', Config::get('cms.loadModules')) + && Str::startsWith($package, 'theme-') + && Theme::exists(Str::after($package, 'theme-')) + ) { + return static::TYPE_THEME; + } + + // Check if a package could be a plugin + if (PluginManager::instance()->exists($package)) { + return static::TYPE_PLUGIN; + } + + return null; + } + /** * Returns the registration method for a compiler type */ diff --git a/modules/system/console/asset/AssetInstall.php b/modules/system/console/asset/AssetInstall.php index 6ecb911c33..c7da65ec4a 100644 --- a/modules/system/console/asset/AssetInstall.php +++ b/modules/system/console/asset/AssetInstall.php @@ -8,6 +8,8 @@ use System\Classes\Asset\PackageJson; use System\Classes\Asset\PackageManager; use System\Classes\PluginManager; +use System\Console\Asset\Exceptions\PackageIgnoredException; +use System\console\asset\exceptions\PackageNotConfiguredException; use System\Console\Asset\Exceptions\PackageNotFoundException; use Winter\Storm\Console\Command; use Winter\Storm\Support\Facades\Config; @@ -128,39 +130,34 @@ protected function getRequestedAndRegisteredPackages(): array continue; } - // Check if package could be a module (but explicitly ignore core Winter modules) - if (Str::startsWith($package, 'module-') && !in_array($package, ['system', 'backend', 'cms'])) { - $compilableAssets->registerPackage( - $package, - base_path('modules/' . Str::after($package, 'module-') . '/' . $this->configFile), - $this->assetType - ); - continue; - } - - // Check if package could be a theme - if ( - $cmsEnabled - && Str::startsWith($package, 'theme-') - && Theme::exists(Str::after($package, 'theme-')) - ) { - $theme = Theme::load(Str::after($package, 'theme-')); - $compilableAssets->registerPackage( - $package, - $theme->getPath() . '/' . $this->configFile, - $this->assetType - ); - continue; - } - - // Check if a package could be a plugin - if (PluginManager::instance()->exists($package)) { - $compilableAssets->registerPackage( - $package, - PluginManager::instance()->getPluginPath($package) . '/' . $this->configFile, - $this->assetType - ); - continue; + switch (PackageManager::instance()->getPackageTypeFromName($package)) { + case PackageManager::TYPE_MODULE: + $compilableAssets->registerPackage( + $package, + base_path('modules/' . Str::after($package, 'module-') . '/' . $this->configFile), + $this->assetType + ); + break; + case PackageManager::TYPE_THEME: + $theme = Theme::load(Str::after($package, 'theme-')); + $compilableAssets->registerPackage( + $package, + $theme->getPath() . '/' . $this->configFile, + $this->assetType + ); + break; + case PackageManager::TYPE_PLUGIN: + $compilableAssets->registerPackage( + $package, + PluginManager::instance()->getPluginPath($package) . '/' . $this->configFile, + $this->assetType + ); + break; + case null: + throw new PackageNotFoundException(sprintf( + 'The package `%s` does not exist.', + $package + )); } } @@ -197,67 +194,103 @@ protected function processPackages(array $registeredPackages, PackageJson $packa { // Check if the user requested a specific package for install $requestedPackage = strtolower($this->argument('assetPackage')); - $foundRequestedPackage = !$requestedPackage; - if (!$foundRequestedPackage) { - foreach ($registeredPackages as $name => $package) { - if ($requestedPackage === $name) { - $foundRequestedPackage = true; + if ($requestedPackage) { + // We did not find the package, exit + if (!isset($registeredPackages[$requestedPackage])) { + if ($detected = PackageManager::instance()->getPackage($requestedPackage)) { + switch (count($detected)) { + case 1: + if ($detected[0]['type'] !== $this->assetType) { + throw new PackageNotConfiguredException(sprintf( + 'The requested package `%s` is only configured for %s. Run `php artisan %s:create %1$s`', + $this->argument('assetPackage'), + $detected[0]['type'], + $this->assetType + )); + } + + if ($detected[0]['ignored']) { + throw new PackageIgnoredException(sprintf( + 'The requested package `%s` is ignored, remove it from package.json to continue', + $this->argument('assetPackage'), + )); + } + break; + case 2: + default: + if (($detected[0]['ignored'] ?? false) || ($detected[1]['ignored'] ?? false)) { + throw new PackageIgnoredException(sprintf( + 'The requested package `%s` is ignored, remove it from package.json to continue', + $this->argument('assetPackage'), + )); + } + break; + } } - } - // We did not find the package, exit - if (!$foundRequestedPackage) { throw new PackageNotFoundException(sprintf( - 'The requested package `%s` could not be found. Try %s:config or check if the package is ignored.', + 'The requested package `%s` could not be found.', $this->argument('assetPackage'), - $this->assetType )); } + + $this->processPackage($packageJson, $requestedPackage, $registeredPackages[$requestedPackage], true); + + return $packageJson; } // Process each found package foreach ($registeredPackages as $name => $package) { - // Normalize package path across OS types - $packagePath = Str::replace(DIRECTORY_SEPARATOR, '/', $package['path']); - // Add the package path to the instance's package.json->workspaces->packages property if not present - if (!$packageJson->hasWorkspace($packagePath) && !$packageJson->hasIgnoredPackage($packagePath)) { - if ( - $requestedPackage === $name - || $this->confirm( - sprintf( - "Detected %s (%s), should it be added to your package.json?", - $name, - $packagePath - ), - true - ) - ) { - $packageJson->addWorkspace($packagePath); - $this->info(sprintf( - 'Adding %s (%s) to the workspaces.packages property in package.json', + $this->processPackage($packageJson, $name, $package); + } + + return $packageJson; + } + + protected function processPackage(PackageJson $packageJson, string $name, array $package, bool $force = false): bool + { + // Normalize package path across OS types + $packagePath = Str::replace(DIRECTORY_SEPARATOR, '/', $package['path']); + // Add the package path to the instance's package.json->workspaces->packages property if not present + if (!$packageJson->hasWorkspace($packagePath) && !$packageJson->hasIgnoredPackage($packagePath)) { + if ( + $force + || $this->confirm( + sprintf( + "Detected %s (%s), should it be added to your package.json?", $name, $packagePath - )); - } else { - $packageJson->addIgnoredPackage($packagePath); - $this->warn( - sprintf('Ignoring %s (%s)', $name, $packagePath) - ); - } - } - - // Detect missing config files and install them - if (!File::exists($package['config'])) { + ), + true + ) + ) { + $packageJson->addWorkspace($packagePath); $this->info(sprintf( - 'No config file found for %s, you should run %s:config', + 'Adding %s (%s) to the workspaces.packages property in package.json', $name, - $this->assetType + $packagePath )); + } else { + $packageJson->addIgnoredPackage($packagePath); + $this->warn( + sprintf('Ignoring %s (%s)', $name, $packagePath) + ); } } - return $packageJson; + // Detect missing config files and install them + if (!File::exists($package['config'])) { + $this->info(sprintf( + 'No config file found for %s, you should run %s:config', + $name, + $this->assetType + )); + + return false; + } + + return true; } /** diff --git a/modules/system/console/asset/exceptions/PackageIgnoredException.php b/modules/system/console/asset/exceptions/PackageIgnoredException.php new file mode 100644 index 0000000000..c81d60db0d --- /dev/null +++ b/modules/system/console/asset/exceptions/PackageIgnoredException.php @@ -0,0 +1,8 @@ + Date: Fri, 20 Sep 2024 16:07:14 +0100 Subject: [PATCH 05/28] Added fixes for silent flag and added disable-tty as an option --- modules/system/console/asset/NpmCommand.php | 10 +++++---- .../system/console/asset/npm/NpmInstall.php | 4 +++- modules/system/console/asset/npm/NpmRun.php | 7 +++++-- .../system/console/asset/npm/NpmUpdate.php | 21 +++++++++++++------ 4 files changed, 29 insertions(+), 13 deletions(-) diff --git a/modules/system/console/asset/NpmCommand.php b/modules/system/console/asset/NpmCommand.php index d3e57b589e..7b8d998eb4 100644 --- a/modules/system/console/asset/NpmCommand.php +++ b/modules/system/console/asset/NpmCommand.php @@ -43,10 +43,12 @@ protected function npmRun(array $command, string $path): int null ); - try { - $process->setTty(true); - } catch (\Throwable $e) { - // This will fail on unsupported systems + if (!$this->option('disable-tty') && !$this->option('silent')) { + try { + $process->setTty(true); + } catch (\Throwable $e) { + // This will fail on unsupported systems + } } return $process->run(function ($status, $stdout) { diff --git a/modules/system/console/asset/npm/NpmInstall.php b/modules/system/console/asset/npm/NpmInstall.php index aff7d90197..ebdac1746b 100644 --- a/modules/system/console/asset/npm/NpmInstall.php +++ b/modules/system/console/asset/npm/NpmInstall.php @@ -24,7 +24,9 @@ class NpmInstall extends NpmCommand {package? : The package name to add configuration for} {npmArgs?* : Arguments to pass through to the "npm" binary} {--npm= : Defines a custom path to the "npm" binary} - {--d|dev : Install packages in devDependencies}'; + {--d|dev : Install packages in devDependencies} + {--s|silent : Silent mode.} + {--disable-tty : Disable tty mode}'; /** * @inheritDoc diff --git a/modules/system/console/asset/npm/NpmRun.php b/modules/system/console/asset/npm/NpmRun.php index d3fa1441fe..ee4a7d7457 100644 --- a/modules/system/console/asset/npm/NpmRun.php +++ b/modules/system/console/asset/npm/NpmRun.php @@ -19,7 +19,8 @@ class NpmRun extends NpmCommand {script : The name of the script to run, as defined in the package.json "scripts" config.} {additionalArgs?* : Arguments to pass through to the script being run.} {--f|production : Runs the script in "production" mode.} - {--s|silent : Silent mode.}'; + {--s|silent : Silent mode.} + {--disable-tty : Disable tty mode}'; /** * @var string The console command description. @@ -49,7 +50,9 @@ public function handle(): int return 1; } - $this->info(sprintf('Running script "%s" in package "%s"', $script, $this->argument('package'))); + if (!$this->option('silent')) { + $this->info(sprintf('Running script "%s" in package "%s"', $script, $this->argument('package'))); + } $command = ($this->argument('additionalArgs')) ?? []; if (count($command)) { diff --git a/modules/system/console/asset/npm/NpmUpdate.php b/modules/system/console/asset/npm/NpmUpdate.php index 74b7a64af0..144a991da3 100644 --- a/modules/system/console/asset/npm/NpmUpdate.php +++ b/modules/system/console/asset/npm/NpmUpdate.php @@ -21,9 +21,12 @@ class NpmUpdate extends NpmCommand * @inheritDoc */ protected $signature = 'npm:update - {package? : The package name to add configuration for} - {npmArgs?* : Arguments to pass through to the "npm" binary} - {--npm= : Defines a custom path to the "npm" binary}'; + {package? : The package name to add configuration for.} + {npmArgs?* : Arguments to pass through to the "npm" binary.} + {--npm= : Defines a custom path to the "npm" binary.} + {--a|save : Tell npm to update package.json.} + {--s|silent : Silent mode.} + {--disable-tty : Disable tty mode}'; /** * @inheritDoc @@ -42,12 +45,18 @@ public function handle(): int array_unshift($command, $this->argument('package')); } + $args = ['npm', 'update']; + + if ($this->option('save')) { + $args[] = '--save'; + } + if (count($command)) { - array_unshift($command, 'npm', 'update', '--'); - } else { - array_unshift($command, 'npm', 'update'); + $args[] = '--'; } + array_unshift($command, ...$args); + return $this->npmRun($command, $package['path'] ?? ''); } } From 8a76ea857a5e4723534f334c67f10f268a8ed46b Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Fri, 20 Sep 2024 16:08:11 +0100 Subject: [PATCH 06/28] Added tests and fixtures --- .../console/asset/npm/NpmInstallTest.php | 216 ++++++++++++++++++ .../tests/console/asset/npm/NpmRunTest.php | 55 +++++ .../tests/console/asset/npm/NpmUpdateTest.php | 111 +++++++++ .../fixtures/themes/npmtest/package.json | 11 + .../fixtures/themes/npmtest/vite.config.mjs | 2 + 5 files changed, 395 insertions(+) create mode 100644 modules/system/tests/console/asset/npm/NpmInstallTest.php create mode 100644 modules/system/tests/console/asset/npm/NpmRunTest.php create mode 100644 modules/system/tests/console/asset/npm/NpmUpdateTest.php create mode 100644 modules/system/tests/fixtures/themes/npmtest/package.json create mode 100644 modules/system/tests/fixtures/themes/npmtest/vite.config.mjs diff --git a/modules/system/tests/console/asset/npm/NpmInstallTest.php b/modules/system/tests/console/asset/npm/NpmInstallTest.php new file mode 100644 index 0000000000..4fefb0d35d --- /dev/null +++ b/modules/system/tests/console/asset/npm/NpmInstallTest.php @@ -0,0 +1,216 @@ +markTestSkipped('This test requires node_modules to be installed'); + } + + // Define some helpful paths + $this->themePath = base_path('modules/system/tests/fixtures/themes/npmtest'); + $this->jsonPath = $this->themePath . '/package.json'; + $this->lockPath = $this->themePath . '/package-lock.json'; + $this->backupPath = $this->themePath . '/package.backup.json'; + + // Add our testing theme because it won't be auto discovered + PackageManager::instance()->registerPackage( + 'theme-npmtest', + $this->themePath . '/vite.config.mjs', + 'vite' + ); + } + + /** + * Test the ability to install a single npm package via artisan + */ + public function testNpmInstallSingle(): void + { + // Validate the package Json does not have dependencies + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayNotHasKey('dependencies', $packageJson); + + // Validate node_modules not found + $this->assertDirectoryDoesNotExist($this->themePath . '/node_modules'); + $this->assertFileNotExists($this->lockPath); + + $this->withPackageJsonRestore(function () { + // Run npm install for a single non-dev package + $this->artisan('npm:install', [ + 'package' => 'theme-npmtest', + 'npmArgs' => ['is-odd'], + '--disable-tty' => true + ]) + ->assertExitCode(0); + + // Validate lock file was generated successfully + $this->assertFileExists($this->lockPath); + + // Validate the contents of package.json + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('dependencies', $packageJson); + $this->assertArrayHasKey('is-odd', $packageJson['dependencies']); + + // Validate that node_modules paths exist + $this->assertDirectoryExists($this->themePath . '/node_modules'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-odd'); + }); + } + + /** + * Test the ability to install multiple npm packages via artisan + */ + public function testNpmInstallMultiple(): void + { + // Validate the package Json does not have dependencies + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayNotHasKey('dependencies', $packageJson); + + // Validate node_modules not found + $this->assertDirectoryDoesNotExist($this->themePath . '/node_modules'); + $this->assertFileNotExists($this->lockPath); + + $this->withPackageJsonRestore(function () { + // Run npm install for multiple non-dev packages + $this->artisan('npm:install', [ + 'package' => 'theme-npmtest', + 'npmArgs' => ['is-odd', 'is-even'], + '--disable-tty' => true + ]) + ->assertExitCode(0); + + // Validate lock file was generated successfully + $this->assertFileExists($this->lockPath); + + // Validate the contents of package.json + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('dependencies', $packageJson); + $this->assertArrayHasKey('is-odd', $packageJson['dependencies']); + $this->assertArrayHasKey('is-even', $packageJson['dependencies']); + + // Validate that node_modules paths exist + $this->assertDirectoryExists($this->themePath . '/node_modules'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-odd'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-even'); + }); + } + + + /** + * Test the ability to install a single dev npm package via artisan + */ + public function testNpmInstallSingleDev(): void + { + // Validate the package Json does not have dependencies + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayNotHasKey('devDependencies', $packageJson); + + // Validate node_modules not found + $this->assertDirectoryDoesNotExist($this->themePath . '/node_modules'); + $this->assertFileNotExists($this->lockPath); + + $this->withPackageJsonRestore(function () { + // Run npm install for a single dev package + $this->artisan('npm:install', [ + 'package' => 'theme-npmtest', + 'npmArgs' => ['is-odd'], + '--dev' => true, + '--disable-tty' => true + ]) + ->assertExitCode(0); + + // Validate lock file was generated successfully + $this->assertFileExists($this->lockPath); + + // Validate the contents of package.json + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('is-odd', $packageJson['devDependencies']); + + // Validate that node_modules paths exist + $this->assertDirectoryExists($this->themePath . '/node_modules'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-odd'); + }); + } + + /** + * Test the ability to install multiple dev npm packages via artisan + */ + public function testNpmInstallMultipleDev(): void + { + // Validate the package Json does not have dependencies + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayNotHasKey('devDependencies', $packageJson); + + // Validate node_modules not found + $this->assertDirectoryDoesNotExist($this->themePath . '/node_modules'); + $this->assertFileNotExists($this->lockPath); + + $this->withPackageJsonRestore(function () { + // Run npm install for multiple dev packages + $this->artisan('npm:install', [ + 'package' => 'theme-npmtest', + 'npmArgs' => ['is-odd', 'is-even'], + '--dev' => true, + '--disable-tty' => true + ]) + ->assertExitCode(0); + + // Validate lock file was generated successfully + $this->assertFileExists($this->lockPath); + + // Validate the contents of package.json + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('is-odd', $packageJson['devDependencies']); + $this->assertArrayHasKey('is-even', $packageJson['devDependencies']); + + // Validate that node_modules paths exist + $this->assertDirectoryExists($this->themePath . '/node_modules'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-odd'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-even'); + }); + } + + /** + * Helper to run test logic and handle restoring package.json file after + */ + protected function withPackageJsonRestore(callable $callback): void + { + copy($this->jsonPath, $this->backupPath); + $callback(); + rename($this->backupPath, $this->jsonPath); + } + + /** + * Cleanup the test theme + */ + public function tearDown(): void + { + if (File::isDirectory($this->themePath . '/node_modules')) { + File::deleteDirectory($this->themePath . '/node_modules'); + } + + foreach ([$this->backupPath, $this->lockPath] as $path) { + if (File::exists($path)) { + File::delete($path); + } + } + + parent::tearDown(); + } +} diff --git a/modules/system/tests/console/asset/npm/NpmRunTest.php b/modules/system/tests/console/asset/npm/NpmRunTest.php new file mode 100644 index 0000000000..51f699dbb0 --- /dev/null +++ b/modules/system/tests/console/asset/npm/NpmRunTest.php @@ -0,0 +1,55 @@ +markTestSkipped('This test requires node_modules to be installed'); + } + + // Add our testing theme because it won't be auto discovered + PackageManager::instance()->registerPackage( + 'theme-npmtest', + base_path('modules/system/tests/fixtures/themes/npmtest/vite.config.mjs'), + 'vite' + ); + } + + /** + * Test the ability to run a npm script via artisan + */ + public function testNpmRunScript(): void + { + $this->artisan('npm:run', [ + 'package' => 'theme-npmtest', + 'script' => 'testScript', + '--disable-tty' => true + ]) + ->expectsOutputToContain('> echo "Winter says $((1+2))"') + ->expectsOutputToContain('Winter says 3') + ->assertExitCode(0); + } + + /** + * Test the error handling of a missing script + */ + public function testNpmRunScriptFailed(): void + { + $this->artisan('npm:run', [ + 'package' => 'theme-npmtest', + 'script' => 'testMissingScript', + '--disable-tty' => true + ]) + ->expectsOutputToContain('Script "testMissingScript" is not defined in package "theme-npmtest"') + ->assertExitCode(1); + } +} diff --git a/modules/system/tests/console/asset/npm/NpmUpdateTest.php b/modules/system/tests/console/asset/npm/NpmUpdateTest.php new file mode 100644 index 0000000000..27c1d61edf --- /dev/null +++ b/modules/system/tests/console/asset/npm/NpmUpdateTest.php @@ -0,0 +1,111 @@ +markTestSkipped('This test requires node_modules to be installed'); + } + + // Define some helpful paths + $this->themePath = base_path('modules/system/tests/fixtures/themes/npmtest'); + $this->jsonPath = $this->themePath . '/package.json'; + $this->lockPath = $this->themePath . '/package-lock.json'; + $this->backupPath = $this->themePath . '/package.backup.json'; + + // Add our testing theme because it won't be auto discovered + PackageManager::instance()->registerPackage( + 'theme-npmtest', + $this->themePath . '/vite.config.mjs', + 'vite' + ); + } + + /** + * Test the ability to install a single npm package via artisan + */ + public function testNpmUpdate(): void + { + // Validate the package Json does not have dependencies + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayNotHasKey('dependencies', $packageJson); + + // Validate node_modules not found + $this->assertDirectoryDoesNotExist($this->themePath . '/node_modules'); + $this->assertFileNotExists($this->lockPath); + + $this->withPackageJsonRestore(function () { + // Update the contents of package.json to include a package at an old patch + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $packageJson['dependencies'] = [ + 'is-odd' => '^3.0.0' + ]; + File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); + + // Run npm update + $this->artisan('npm:update', [ + 'package' => 'theme-npmtest', + '--save' => true, + '--disable-tty' => true + ]) + ->assertExitCode(0); + + // Validate lock file was generated successfully + $this->assertFileExists($this->lockPath); + + // Get the new contents of package.json + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + + // Validate that the package.json contents does not match the old patch value we added above + $this->assertArrayHasKey('dependencies', $packageJson); + $this->assertArrayHasKey('is-odd', $packageJson['dependencies']); + $this->assertNotEquals('^3.0.0', $packageJson['dependencies']['is-odd']); + + // Validate that node_modules paths exist + $this->assertDirectoryExists($this->themePath . '/node_modules'); + $this->assertDirectoryExists($this->themePath . '/node_modules/is-odd'); + }); + } + + /** + * Helper to run test logic and handle restoring package.json file after + */ + protected function withPackageJsonRestore(callable $callback): void + { + copy($this->jsonPath, $this->backupPath); + $callback(); + rename($this->backupPath, $this->jsonPath); + } + + /** + * Cleanup the test theme + */ + public function tearDown(): void + { + if (File::isDirectory($this->themePath . '/node_modules')) { + File::deleteDirectory($this->themePath . '/node_modules'); + } + + foreach ([$this->backupPath, $this->lockPath] as $path) { + if (File::exists($path)) { + File::delete($path); + } + } + + parent::tearDown(); + } +} diff --git a/modules/system/tests/fixtures/themes/npmtest/package.json b/modules/system/tests/fixtures/themes/npmtest/package.json new file mode 100644 index 0000000000..98a117bfe0 --- /dev/null +++ b/modules/system/tests/fixtures/themes/npmtest/package.json @@ -0,0 +1,11 @@ +{ + "type": "module", + "workspaces": { + "packages": [ + "modules/system/tests/fixtures/themes/npmtest" + ] + }, + "scripts": { + "testScript": "echo \"Winter says $((1+2))\"" + } +} diff --git a/modules/system/tests/fixtures/themes/npmtest/vite.config.mjs b/modules/system/tests/fixtures/themes/npmtest/vite.config.mjs new file mode 100644 index 0000000000..0b79ce8fee --- /dev/null +++ b/modules/system/tests/fixtures/themes/npmtest/vite.config.mjs @@ -0,0 +1,2 @@ +import { defineConfig } from 'vite'; +export default defineConfig({}); From 90a0d1793eabf8845c144e2dd49b635aa02c2ce5 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:48:19 +0100 Subject: [PATCH 07/28] Renamed vitetest theme to assettest --- .../fixtures/themes/{vitetest => assettest}/assets/css/theme.css | 0 .../themes/{vitetest => assettest}/assets/javascript/theme.js | 0 .../tests/fixtures/themes/{vitetest => assettest}/vite.config.mjs | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename modules/system/tests/fixtures/themes/{vitetest => assettest}/assets/css/theme.css (100%) rename modules/system/tests/fixtures/themes/{vitetest => assettest}/assets/javascript/theme.js (100%) rename modules/system/tests/fixtures/themes/{vitetest => assettest}/vite.config.mjs (100%) diff --git a/modules/system/tests/fixtures/themes/vitetest/assets/css/theme.css b/modules/system/tests/fixtures/themes/assettest/assets/css/theme.css similarity index 100% rename from modules/system/tests/fixtures/themes/vitetest/assets/css/theme.css rename to modules/system/tests/fixtures/themes/assettest/assets/css/theme.css diff --git a/modules/system/tests/fixtures/themes/vitetest/assets/javascript/theme.js b/modules/system/tests/fixtures/themes/assettest/assets/javascript/theme.js similarity index 100% rename from modules/system/tests/fixtures/themes/vitetest/assets/javascript/theme.js rename to modules/system/tests/fixtures/themes/assettest/assets/javascript/theme.js diff --git a/modules/system/tests/fixtures/themes/vitetest/vite.config.mjs b/modules/system/tests/fixtures/themes/assettest/vite.config.mjs similarity index 100% rename from modules/system/tests/fixtures/themes/vitetest/vite.config.mjs rename to modules/system/tests/fixtures/themes/assettest/vite.config.mjs From e7e770d253f6ac9c5bf6f8f8c006f79dff1ad0cc Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:48:49 +0100 Subject: [PATCH 08/28] Added fix to PackageJson to handle malformed json files --- modules/system/classes/asset/PackageJson.php | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/modules/system/classes/asset/PackageJson.php b/modules/system/classes/asset/PackageJson.php index be6469c10b..7e4900993c 100644 --- a/modules/system/classes/asset/PackageJson.php +++ b/modules/system/classes/asset/PackageJson.php @@ -19,7 +19,7 @@ class PackageJson /** * The contents of the package.json being modified */ - protected array $data; + protected array $data = []; /** * Create a new instance with optional path, loads file if file already exists @@ -27,9 +27,13 @@ class PackageJson public function __construct( protected ?string $path = null ) { - $this->data = File::exists($this->path) - ? json_decode(File::get($this->path), JSON_OBJECT_AS_ARRAY) - : []; + if (File::exists($this->path)) { + $raw = File::get($this->path); + if (!json_validate($raw)) { + throw new \JsonException('The contents of the file "' . $this->path . '" is not valid json.'); + } + $this->data = json_decode($raw, JSON_OBJECT_AS_ARRAY); + } } /** From 3d3b2bfb7038a594e3fa9225d7b3b582da74f26b Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:50:05 +0100 Subject: [PATCH 09/28] Added caching of package.json file and dynamic ignored loading to enable testing --- .../system/classes/asset/PackageManager.php | 45 +++++++++++++------ 1 file changed, 31 insertions(+), 14 deletions(-) diff --git a/modules/system/classes/asset/PackageManager.php b/modules/system/classes/asset/PackageManager.php index 829c03b052..231cc55baa 100644 --- a/modules/system/classes/asset/PackageManager.php +++ b/modules/system/classes/asset/PackageManager.php @@ -31,7 +31,7 @@ class PackageManager /** * The filename that stores the package definition. */ - protected string $packageJson = 'package.json'; + protected PackageJson $packageJson; /** * @var array> List of package types and registration methods @@ -60,6 +60,8 @@ class PackageManager */ public function init(): void { + $this->setPackageJsonPath(base_path('package.json')); + $packagePaths = []; /* @@ -187,12 +189,14 @@ public static function registerCallback(callable $callback): void /** * Calls the deferred callbacks. */ - public function fireCallbacks(): void + public function fireCallbacks(): static { // Call callbacks foreach (static::$callbacks as $callback) { $callback($this); } + + return $this; } /** @@ -210,6 +214,10 @@ public function getPackages(string $type, bool $includeIgnored = false): array { $packages = $this->packages[$type] ?? []; + foreach ($packages as $index => $package) { + $packages[$index]['ignored'] = $this->isPackageIgnored($package['path']); + } + ksort($packages); if (!$includeIgnored) { @@ -227,9 +235,13 @@ public function getPackages(string $type, bool $includeIgnored = false): array public function hasPackage(string $name, bool $includeIgnored = false): bool { foreach ($this->packages ?? [] as $packages) { - foreach ($packages as $packageName => $config) { - if (($name === $packageName) && (!$config['ignored'] || $includeIgnored)) { - return true; + foreach ($packages as $packageName => $package) { + if ($name === $packageName) { + if ((!$this->isPackageIgnored($package['path']) || $includeIgnored)) { + return true; + } + + return false; } } } @@ -244,9 +256,11 @@ public function getPackage(string $name, bool $includeIgnored = false): array { $results = []; foreach ($this->packages ?? [] as $type => $packages) { - foreach ($packages as $packageName => $config) { - if (($name === $packageName) && (!$config['ignored'] || $includeIgnored)) { - $results[] = $config + ['type' => $type]; + foreach ($packages as $packageName => $package) { + if (($name === $packageName)) { + if (!$this->isPackageIgnored($package['path']) || $includeIgnored) { + $results[] = $package + ['type' => $type]; + } } } } @@ -310,7 +324,7 @@ public function registerPackage(string $name, string $path, string $type = 'mix' )); } - $package = "$path/{$this->packageJson}"; + $package = $path . '/package.json'; $config = $path . DIRECTORY_SEPARATOR . $configFile; // Check for any existing package that already registers the given compilable config path @@ -329,8 +343,7 @@ public function registerPackage(string $name, string $path, string $type = 'mix' $this->packages[$type][$name] = [ 'path' => $path, 'package' => $package, - 'config' => $config, - 'ignored' => $this->isPackageIgnored($path), + 'config' => $config ]; } @@ -358,6 +371,12 @@ public function getPackageTypeFromName(string $package): ?string return null; } + public function setPackageJsonPath(string $packageJsonPath): static + { + $this->packageJson = new PackageJson($packageJsonPath); + return $this; + } + /** * Returns the registration method for a compiler type */ @@ -371,8 +390,6 @@ protected function getRegistrationMethod(string $type): string */ protected function isPackageIgnored(string $packagePath): bool { - // Load the main package.json for the project - $packageJson = new PackageJson(base_path($this->packageJson)); - return $packageJson->hasIgnoredPackage($packagePath); + return $this->packageJson->hasIgnoredPackage($packagePath); } } From b5b3b976cfd32883da2deb008d6d90f7a5ba4296 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:51:38 +0100 Subject: [PATCH 10/28] Added simplification and support for testing to asset:install commands --- modules/system/console/asset/AssetInstall.php | 255 +++++++++++------- .../system/console/asset/mix/MixInstall.php | 10 +- .../system/console/asset/vite/ViteInstall.php | 10 +- 3 files changed, 165 insertions(+), 110 deletions(-) diff --git a/modules/system/console/asset/AssetInstall.php b/modules/system/console/asset/AssetInstall.php index c7da65ec4a..901299d632 100644 --- a/modules/system/console/asset/AssetInstall.php +++ b/modules/system/console/asset/AssetInstall.php @@ -32,9 +32,9 @@ abstract class AssetInstall extends Command ]; /** - * The NPM command to run. + * Path to package json, if null use base_path. */ - protected string $npmCommand = 'install'; + protected ?string $packageJsonPath = null; /** * Type of asset to be installed, @see PackageManager @@ -47,17 +47,21 @@ abstract class AssetInstall extends Command protected string $configFile; /** - * The required packages for this compiler + * The required dependencies for this compiler */ - protected array $requiredPackages = []; + protected array $requiredDependencies = []; /** * Execute the console command. */ public function handle(): int { - if ($this->option('npm')) { - $this->npmPath = $this->option('npm', 'npm'); + if ($npmPath = $this->option('npm')) { + if (!File::exists($npmPath) || !is_executable($npmPath)) { + $this->error('The supplied --npm path does not exist or is not executable.'); + return 1; + } + $this->npmPath = $npmPath; } if (!version_compare($this->getNpmVersion(), '7', '>')) { @@ -65,89 +69,103 @@ public function handle(): int return 1; } - [$requestedPackages, $registeredPackages] = $this->getRequestedAndRegisteredPackages(); + // If a custom path is passed, then validate it + if ($packageJsonPath = $this->option('package-json')) { + // If this is not an absolute path, then make it relative + if (!str_starts_with($packageJsonPath, '/')) { + $packageJsonPath = base_path($packageJsonPath); + } - if (!count($registeredPackages)) { - if (count($requestedPackages)) { - $this->error('No registered packages matched the requested packages for installation.'); + if (!File::exists($packageJsonPath)) { + $this->error('The supplied --package-json path does not exist.'); return 1; - } else { - $this->info('No packages registered for mixing.'); - return 0; } + + $this->packageJsonPath = $packageJsonPath; } - // Load the main package.json for the project - $packageJsonPath = base_path('package.json'); + // Get any packages the user has requested + $requestedPackages = $this->argument('assetPackage') ?: []; + + $registeredPackages = $this->getRegisteredPackages($requestedPackages); + + if (!$registeredPackages) { + if ($requestedPackages) { + $this->error('No registered packages matched the requested packages for installation.'); + return 1; + } + + $this->info('No packages registered for mixing.'); + return 0; + } // Get base package.json - $packageJson = new PackageJson($packageJsonPath); + $packageJson = new PackageJson($this->packageJsonPath ?? base_path('package.json')); + // Ensure asset compiling packages are set in package.json, then save - $this->validateRequirePackagesPresent($packageJson) + $this->validateRequireDependenciesPresent($packageJson) ->save(); + // Process compilable asset packages, then save $this->processPackages($registeredPackages, $packageJson) ->save(); - // Ensure separation between package.json modification messages and rest of output - $this->info(''); + if (!$this->option('no-install')) { + // Ensure separation between package.json modification messages and rest of output + $this->info(''); - if ($this->installPackageDeps() !== 0) { - $this->error("Unable to {$this->terms['complete']} dependencies."); - return 1; - } + if ($this->runNpmInstall() !== 0) { + $this->error("Unable to {$this->terms['complete']} dependencies."); + return 1; + } - $this->info("Dependencies successfully {$this->terms['completed']}!"); + $this->info("Dependencies successfully {$this->terms['completed']}!"); + } return 0; } - protected function getRequestedAndRegisteredPackages(): array + protected function getRegisteredPackages(array $requestedPackages = []): array { - $compilableAssets = PackageManager::instance(); - $compilableAssets->fireCallbacks(); + $packageManager = $this->getPackageManager(); - $registeredPackages = $compilableAssets->getPackages($this->assetType); - $requestedPackages = $this->option('package') ?: []; + $registeredPackages = $packageManager->getPackages($this->assetType, true); // Normalize the requestedPackages option - if (count($requestedPackages)) { - foreach ($requestedPackages as &$name) { - $name = strtolower($name); - } - unset($name); - } + $requestedPackages = array_map(fn ($name) => strtolower($name), $requestedPackages); // Filter the registered packages to only include requested packages if (count($requestedPackages) && count($registeredPackages)) { - $availablePackages = array_keys($registeredPackages); $cmsEnabled = in_array('Cms', Config::get('cms.loadModules')); // Autogenerate config files for packages that don't exist but can be autodiscovered foreach ($requestedPackages as $package) { // Check if the package is already registered - if (in_array($package, $availablePackages)) { + if (isset($registeredPackages[$package])) { continue; } - switch (PackageManager::instance()->getPackageTypeFromName($package)) { + switch ($packageManager->getPackageTypeFromName($package)) { case PackageManager::TYPE_MODULE: - $compilableAssets->registerPackage( + $packageManager->registerPackage( $package, base_path('modules/' . Str::after($package, 'module-') . '/' . $this->configFile), $this->assetType ); break; case PackageManager::TYPE_THEME: + if (!$cmsEnabled) { + break; + } $theme = Theme::load(Str::after($package, 'theme-')); - $compilableAssets->registerPackage( + $packageManager->registerPackage( $package, $theme->getPath() . '/' . $this->configFile, $this->assetType ); break; case PackageManager::TYPE_PLUGIN: - $compilableAssets->registerPackage( + $packageManager->registerPackage( $package, PluginManager::instance()->getPluginPath($package) . '/' . $this->configFile, $this->assetType @@ -162,7 +180,7 @@ protected function getRequestedAndRegisteredPackages(): array } // Get an updated list of packages including any newly added packages - $registeredPackages = $compilableAssets->getPackages($this->assetType); + $registeredPackages = $packageManager->getPackages($this->assetType, true); // Filter the registered packages to only deal with the requested packages foreach (array_keys($registeredPackages) as $name) { @@ -172,18 +190,18 @@ protected function getRequestedAndRegisteredPackages(): array } } - return [$requestedPackages, $registeredPackages]; + return $registeredPackages; } - protected function validateRequirePackagesPresent(PackageJson $packageJson): PackageJson + protected function validateRequireDependenciesPresent(PackageJson $packageJson): PackageJson { // Check to see if required packages are already present as a dependency - foreach ($this->requiredPackages as $package => $version) { + foreach ($this->requiredDependencies as $dependency => $version) { if ( - !$packageJson->hasDependency($package) - && $this->confirm($package . ' was not found as a dependency in package.json, would you like to add it?', true) + !$packageJson->hasDependency($dependency) + && $this->confirm($dependency . ' was not found as a dependency in package.json, would you like to add it?', true) ) { - $packageJson->addDependency($package, $version, dev: true); + $packageJson->addDependency($dependency, $version, dev: true); } } @@ -193,50 +211,51 @@ protected function validateRequirePackagesPresent(PackageJson $packageJson): Pac protected function processPackages(array $registeredPackages, PackageJson $packageJson): PackageJson { // Check if the user requested a specific package for install - $requestedPackage = strtolower($this->argument('assetPackage')); - - if ($requestedPackage) { - // We did not find the package, exit - if (!isset($registeredPackages[$requestedPackage])) { - if ($detected = PackageManager::instance()->getPackage($requestedPackage)) { - switch (count($detected)) { - case 1: - if ($detected[0]['type'] !== $this->assetType) { - throw new PackageNotConfiguredException(sprintf( - 'The requested package `%s` is only configured for %s. Run `php artisan %s:create %1$s`', - $this->argument('assetPackage'), - $detected[0]['type'], - $this->assetType - )); - } - - if ($detected[0]['ignored']) { - throw new PackageIgnoredException(sprintf( - 'The requested package `%s` is ignored, remove it from package.json to continue', - $this->argument('assetPackage'), - )); - } - break; - case 2: - default: - if (($detected[0]['ignored'] ?? false) || ($detected[1]['ignored'] ?? false)) { - throw new PackageIgnoredException(sprintf( - 'The requested package `%s` is ignored, remove it from package.json to continue', - $this->argument('assetPackage'), - )); - } - break; + if ($requestedPackages = array_map(fn ($name) => strtolower($name), $this->argument('assetPackage'))) { + $packageManager = $this->getPackageManager(); + foreach ($requestedPackages as $requestedPackage) { + // We did not find the package, exit + if (!isset($registeredPackages[$requestedPackage])) { + if ($detected = $packageManager->getPackage($requestedPackage, true)) { + switch (count($detected)) { + case 1: + if ($detected[0]['type'] !== $this->assetType) { + throw new PackageNotConfiguredException(sprintf( + 'The requested package `%s` is only configured for %s. Run `php artisan %s:create %1$s`', + $requestedPackage, + $detected[0]['type'], + $this->assetType + )); + } + + if ($detected[0]['ignored']) { + throw new PackageIgnoredException(sprintf( + 'The requested package `%s` is ignored, remove it from package.json to continue', + $requestedPackage, + )); + } + break; + case 2: + default: + if (($detected[0]['ignored'] ?? false) || ($detected[1]['ignored'] ?? false)) { + throw new PackageIgnoredException(sprintf( + 'The requested package `%s` is ignored, remove it from package.json to continue', + $requestedPackage, + )); + } + break; + } } + + throw new PackageNotFoundException(sprintf( + 'The requested package `%s` could not be found.', + $requestedPackage, + )); } - throw new PackageNotFoundException(sprintf( - 'The requested package `%s` could not be found.', - $this->argument('assetPackage'), - )); + $this->processPackage($packageJson, $requestedPackage, $registeredPackages[$requestedPackage], true); } - $this->processPackage($packageJson, $requestedPackage, $registeredPackages[$requestedPackage], true); - return $packageJson; } @@ -252,6 +271,28 @@ protected function processPackage(PackageJson $packageJson, string $name, array { // Normalize package path across OS types $packagePath = Str::replace(DIRECTORY_SEPARATOR, '/', $package['path']); + + // Nicely report if the package is already in the workspace + if ($packageJson->hasWorkspace($packagePath)) { + $this->warn(sprintf( + 'Package %s (%s) is already included in workspaces.packages.', + $name, + $packagePath + )); + + return true; + } + + if ($packageJson->hasIgnoredPackage($packagePath)) { + $this->warn(sprintf( + 'The requested package %s (%s) is ignored, remove it from package.json to continue.', + $name, + $packagePath + )); + + return true; + } + // Add the package path to the instance's package.json->workspaces->packages property if not present if (!$packageJson->hasWorkspace($packagePath) && !$packageJson->hasIgnoredPackage($packagePath)) { if ( @@ -279,7 +320,7 @@ protected function processPackage(PackageJson $packageJson, string $name, array } } - // Detect missing config files and install them + // Detect missing config files and provide feedback if (!File::exists($package['config'])) { $this->info(sprintf( 'No config file found for %s, you should run %s:config', @@ -296,18 +337,20 @@ protected function processPackage(PackageJson $packageJson, string $name, array /** * Installs the dependencies for the given package. */ - protected function installPackageDeps(): int + protected function runNpmInstall(): int { - $command = []; - array_unshift($command, 'npm', $this->npmCommand); - - $process = new Process($command, base_path(), null, null, null); - - // Attempt to set tty mode, catch and warn with the exception message if unsupported - try { - $process->setTty(true); - } catch (\Throwable $e) { - $this->warn($e->getMessage()); + $process = new Process( + command: [$this->npmPath, 'install'], + cwd: $this->packageJsonPath ? dirname($this->packageJsonPath) : base_path(), + timeout: null + ); + + if (!$this->option('disable-tty')) { + try { + $process->setTty(true); + } catch (\Throwable $e) { + // This will fail on unsupported systems + } } try { @@ -321,10 +364,18 @@ protected function installPackageDeps(): int return 1; } + } - $this->info(''); + protected function getPackageManager(): PackageManager + { + // Flush the instance + $packageManager = PackageManager::instance()->fireCallbacks(); + // Ensure the instance follows any custom package.json + if ($this->packageJsonPath) { + $packageManager->setPackageJsonPath($this->packageJsonPath); + } - return $process->getExitCode(); + return $packageManager; } /** @@ -332,7 +383,7 @@ protected function installPackageDeps(): int */ protected function getNpmVersion(): string { - $process = new Process(['npm', '--version']); + $process = new Process([$this->npmPath, '--version']); $process->run(); return $process->getOutput(); } diff --git a/modules/system/console/asset/mix/MixInstall.php b/modules/system/console/asset/mix/MixInstall.php index b0f1b52a92..272ad6852a 100644 --- a/modules/system/console/asset/mix/MixInstall.php +++ b/modules/system/console/asset/mix/MixInstall.php @@ -13,9 +13,11 @@ class MixInstall extends AssetInstall * @var string The name and signature of this command. */ protected $signature = 'mix:install - {assetPackage? : The asset package name to install} - {--npm= : Defines a custom path to the "npm" binary} - {--p|package=* : Defines one or more packages to install}'; + {assetPackage?* : The asset package name to install.} + {--no-install : Tells Winter not to run npm install after config update.} + {--npm= : Defines a custom path to the "npm" binary.} + {--d|disable-tty : Disable tty mode.} + {--p|package-json= : Defines a custom path to "package.json" file. Must be above the workspace path.}'; /** * @var string The console command description. @@ -35,7 +37,7 @@ class MixInstall extends AssetInstall /** * The required packages for this compiler */ - protected array $requiredPackages = [ + protected array $requiredDependencies = [ 'laravel-mix' => '^6.0.41', ]; } diff --git a/modules/system/console/asset/vite/ViteInstall.php b/modules/system/console/asset/vite/ViteInstall.php index 683d085622..c7355a1096 100644 --- a/modules/system/console/asset/vite/ViteInstall.php +++ b/modules/system/console/asset/vite/ViteInstall.php @@ -15,9 +15,11 @@ class ViteInstall extends AssetInstall * @var string The name and signature of this command. */ protected $signature = 'vite:install - {assetPackage? : The asset package name to install} - {--npm= : Defines a custom path to the "npm" binary} - {--p|package=* : Defines one or more packages to install}'; + {assetPackage?* : The asset package name to install.} + {--no-install : Tells Winter not to run npm install after config update.} + {--npm= : Defines a custom path to the "npm" binary.} + {--d|disable-tty : Disable tty mode.} + {--p|package-json= : Defines a custom path to "package.json" file. Must be above the workspace path.}'; /** * @var string The console command description. @@ -37,7 +39,7 @@ class ViteInstall extends AssetInstall /** * The required packages for this compiler */ - protected array $requiredPackages = [ + protected array $requiredDependencies = [ 'vite' => '^5.2.11', 'laravel-vite-plugin' => '^1.0.4', ]; From d27911fbac5381465ca541613a62197b77e9a71c Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:52:08 +0100 Subject: [PATCH 11/28] Added catch used by original command --- modules/system/console/asset/NpmCommand.php | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/modules/system/console/asset/NpmCommand.php b/modules/system/console/asset/NpmCommand.php index 7b8d998eb4..220a42befd 100644 --- a/modules/system/console/asset/NpmCommand.php +++ b/modules/system/console/asset/NpmCommand.php @@ -2,6 +2,7 @@ namespace System\Console\Asset; +use Symfony\Component\Process\Exception\ProcessSignaledException; use Symfony\Component\Process\Process; use System\Classes\Asset\PackageJson; use System\Classes\Asset\PackageManager; @@ -46,6 +47,12 @@ protected function npmRun(array $command, string $path): int if (!$this->option('disable-tty') && !$this->option('silent')) { try { $process->setTty(true); + } catch (ProcessSignaledException $e) { + if (extension_loaded('pcntl') && $e->getSignal() !== SIGINT) { + throw $e; + } + + return 1; } catch (\Throwable $e) { // This will fail on unsupported systems } From 4b53b50e0b2f33bcac5c7e8eaab3e132609295b1 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:52:32 +0100 Subject: [PATCH 12/28] Removed incorrect alias --- modules/system/console/asset/npm/NpmInstall.php | 7 ------- 1 file changed, 7 deletions(-) diff --git a/modules/system/console/asset/npm/NpmInstall.php b/modules/system/console/asset/npm/NpmInstall.php index ebdac1746b..b3b0526cdc 100644 --- a/modules/system/console/asset/npm/NpmInstall.php +++ b/modules/system/console/asset/npm/NpmInstall.php @@ -28,13 +28,6 @@ class NpmInstall extends NpmCommand {--s|silent : Silent mode.} {--disable-tty : Disable tty mode}'; - /** - * @inheritDoc - */ - public $replaces = [ - 'mix:update' - ]; - public function handle(): int { $command = ($this->argument('npmArgs')) ?? []; From f0dcdf77ac03548029f3ccb38fc5ee662b05fade Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:53:12 +0100 Subject: [PATCH 13/28] Added fix for theme rename --- .../tests/console/asset/vite/ViteCompileTest.php | 12 ++++++------ .../system/tests/fixtures/npm/package-vitetheme.json | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/modules/system/tests/console/asset/vite/ViteCompileTest.php b/modules/system/tests/console/asset/vite/ViteCompileTest.php index 06ed8805e3..1ace48758a 100644 --- a/modules/system/tests/console/asset/vite/ViteCompileTest.php +++ b/modules/system/tests/console/asset/vite/ViteCompileTest.php @@ -22,11 +22,11 @@ public function setUp(): void $this->markTestSkipped('This test requires the vite package to be installed'); } - $this->themePath = base_path('modules/system/tests/fixtures/themes/vitetest'); + $this->themePath = base_path('modules/system/tests/fixtures/themes/assettest'); // Add our testing theme because it won't be auto discovered PackageManager::instance()->registerPackage( - 'theme-vitetest', + 'theme-assettest', $this->themePath . '/vite.config.mjs', 'vite' ); @@ -36,7 +36,7 @@ public function testThemeCompile(): void { // Run the vite:compile command $this->artisan('vite:compile', [ - 'theme-vitetest', + 'theme-assettest', '--manifest' => 'modules/system/tests/fixtures/npm/package-vitetheme.json', '--silent' => true ])->assertExitCode(0); @@ -73,7 +73,7 @@ public function testThemeCompileFailed(): void // Run the vite:compile command $this->artisan('vite:compile', [ - 'theme-vitetest', + 'theme-assettest', '--manifest' => 'modules/system/tests/fixtures/npm/package-vitetheme.json', '--disable-tty' => true ]) @@ -96,7 +96,7 @@ public function testThemeCompileWarning(): void // Run the vite:compile command $this->artisan('vite:compile', [ - 'theme-vitetest', + 'theme-assettest', '--manifest' => 'modules/system/tests/fixtures/npm/package-vitetheme.json', '--disable-tty' => true ]) @@ -112,7 +112,7 @@ public function testThemeCompileWarning(): void public function tearDown(): void { - File::deleteDirectory('modules/system/tests/fixtures/themes/vitetest/public'); + File::deleteDirectory('modules/system/tests/fixtures/themes/assettest/public'); parent::tearDown(); } } diff --git a/modules/system/tests/fixtures/npm/package-vitetheme.json b/modules/system/tests/fixtures/npm/package-vitetheme.json index 7eb3504b86..b50becfa6b 100644 --- a/modules/system/tests/fixtures/npm/package-vitetheme.json +++ b/modules/system/tests/fixtures/npm/package-vitetheme.json @@ -2,7 +2,7 @@ "type": "module", "workspaces": { "packages": [ - "modules/system/tests/fixtures/themes/vitetest" + "modules/system/tests/fixtures/themes/assettest" ] }, "devDependencies": { From 8bca2d1e873c5fe324d519935ff9a56d7681b3fd Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:54:01 +0100 Subject: [PATCH 14/28] Added trait for preserving package.json between tests logic --- .../tests/console/asset/NpmTestTrait.php | 19 +++++++++++++++++++ .../console/asset/npm/NpmInstallTest.php | 13 +++---------- .../tests/console/asset/npm/NpmUpdateTest.php | 13 +++---------- 3 files changed, 25 insertions(+), 20 deletions(-) create mode 100644 modules/system/tests/console/asset/NpmTestTrait.php diff --git a/modules/system/tests/console/asset/NpmTestTrait.php b/modules/system/tests/console/asset/NpmTestTrait.php new file mode 100644 index 0000000000..a1ce5d677e --- /dev/null +++ b/modules/system/tests/console/asset/NpmTestTrait.php @@ -0,0 +1,19 @@ +jsonPath, $this->backupPath); + $callback(); + rename($this->backupPath, $this->jsonPath); + } +} diff --git a/modules/system/tests/console/asset/npm/NpmInstallTest.php b/modules/system/tests/console/asset/npm/NpmInstallTest.php index 4fefb0d35d..611ab20df7 100644 --- a/modules/system/tests/console/asset/npm/NpmInstallTest.php +++ b/modules/system/tests/console/asset/npm/NpmInstallTest.php @@ -4,10 +4,13 @@ use System\Classes\Asset\PackageManager; use System\Tests\Bootstrap\TestCase; +use System\Tests\Console\Asset\NpmTestTrait; use Winter\Storm\Support\Facades\File; class NpmInstallTest extends TestCase { + use NpmTestTrait; + protected string $themePath; protected string $jsonPath; protected string $lockPath; @@ -186,16 +189,6 @@ public function testNpmInstallMultipleDev(): void }); } - /** - * Helper to run test logic and handle restoring package.json file after - */ - protected function withPackageJsonRestore(callable $callback): void - { - copy($this->jsonPath, $this->backupPath); - $callback(); - rename($this->backupPath, $this->jsonPath); - } - /** * Cleanup the test theme */ diff --git a/modules/system/tests/console/asset/npm/NpmUpdateTest.php b/modules/system/tests/console/asset/npm/NpmUpdateTest.php index 27c1d61edf..38b63d9827 100644 --- a/modules/system/tests/console/asset/npm/NpmUpdateTest.php +++ b/modules/system/tests/console/asset/npm/NpmUpdateTest.php @@ -4,10 +4,13 @@ use System\Classes\Asset\PackageManager; use System\Tests\Bootstrap\TestCase; +use System\Tests\Console\Asset\NpmTestTrait; use Winter\Storm\Support\Facades\File; class NpmUpdateTest extends TestCase { + use NpmTestTrait; + protected string $themePath; protected string $jsonPath; protected string $lockPath; @@ -81,16 +84,6 @@ public function testNpmUpdate(): void }); } - /** - * Helper to run test logic and handle restoring package.json file after - */ - protected function withPackageJsonRestore(callable $callback): void - { - copy($this->jsonPath, $this->backupPath); - $callback(); - rename($this->backupPath, $this->jsonPath); - } - /** * Cleanup the test theme */ From 9c5394cbf60dcd37e09999a71b1f24526d91dcd0 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:54:28 +0100 Subject: [PATCH 15/28] Updated eslintignore for test theme change --- modules/system/.eslintignore | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/system/.eslintignore b/modules/system/.eslintignore index 8cdcdeed59..cc2943407d 100644 --- a/modules/system/.eslintignore +++ b/modules/system/.eslintignore @@ -13,4 +13,4 @@ assets/vendor # Ignore test fixtures tests/js tests/fixtures/themes/test/assets/js -tests/fixtures/themes/vitetest/assets/javascript +tests/fixtures/themes/assettest/assets/javascript From 914cbea03449c74fc3dc0f214f446b1f8724a4be Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:54:59 +0100 Subject: [PATCH 16/28] Added test fixtures --- modules/system/tests/fixtures/themes/assettest/winter.mix.js | 4 ++++ modules/system/tests/fixtures/themes/npmtest/winter.mix.js | 2 ++ modules/system/tests/package.json | 3 +++ 3 files changed, 9 insertions(+) create mode 100644 modules/system/tests/fixtures/themes/assettest/winter.mix.js create mode 100644 modules/system/tests/fixtures/themes/npmtest/winter.mix.js create mode 100644 modules/system/tests/package.json diff --git a/modules/system/tests/fixtures/themes/assettest/winter.mix.js b/modules/system/tests/fixtures/themes/assettest/winter.mix.js new file mode 100644 index 0000000000..6f6731ca3a --- /dev/null +++ b/modules/system/tests/fixtures/themes/assettest/winter.mix.js @@ -0,0 +1,4 @@ +const mix = require('laravel-mix'); +mix.setPublicPath(__dirname); + +mix.js('assets/javascript/theme.js', 'dist/javascript/theme.js'); diff --git a/modules/system/tests/fixtures/themes/npmtest/winter.mix.js b/modules/system/tests/fixtures/themes/npmtest/winter.mix.js new file mode 100644 index 0000000000..ed859bfc52 --- /dev/null +++ b/modules/system/tests/fixtures/themes/npmtest/winter.mix.js @@ -0,0 +1,2 @@ +const mix = require('laravel-mix'); +mix.setPublicPath(__dirname); diff --git a/modules/system/tests/package.json b/modules/system/tests/package.json new file mode 100644 index 0000000000..bfc9f50eea --- /dev/null +++ b/modules/system/tests/package.json @@ -0,0 +1,3 @@ +{ + "name": "winter-test-root" +} From e323a97514a4c11d553c56936a21ea414144b60f Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 17:55:23 +0100 Subject: [PATCH 17/28] Added tests for asset install commands --- .../console/asset/mix/MixInstallTest.php | 193 +++++++++++++++++ .../console/asset/vite/ViteInstallTest.php | 201 ++++++++++++++++++ 2 files changed, 394 insertions(+) create mode 100644 modules/system/tests/console/asset/mix/MixInstallTest.php create mode 100644 modules/system/tests/console/asset/vite/ViteInstallTest.php diff --git a/modules/system/tests/console/asset/mix/MixInstallTest.php b/modules/system/tests/console/asset/mix/MixInstallTest.php new file mode 100644 index 0000000000..332fcf64bc --- /dev/null +++ b/modules/system/tests/console/asset/mix/MixInstallTest.php @@ -0,0 +1,193 @@ +markTestSkipped('This test requires node_modules to be installed'); + } + + // Define some helpful paths + $this->fixturePath = base_path('modules/system/tests'); + $this->jsonPath = $this->fixturePath . '/package.json'; + $this->lockPath = $this->fixturePath . '/package-lock.json'; + $this->backupPath = $this->fixturePath . '/package.backup.json'; + + // Add our testing theme because it won't be auto discovered + PackageManager::instance()->registerPackage( + 'theme-assettest', + base_path('modules/system/tests/fixtures/themes/assettest/winter.mix.js'), + 'mix' + ); + PackageManager::instance()->registerPackage( + 'theme-npmtest', + base_path('modules/system/tests/fixtures/themes/npmtest/winter.mix.js'), + 'mix' + ); + } + + public function testMixInstallSinglePackage(): void + { + $this->withPackageJsonRestore(function () { + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->assertExitCode(0); + + $this->assertFileExists($this->jsonPath); + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('laravel-mix', $packageJson['devDependencies']); + + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayHasKey('packages', $packageJson['workspaces']); + $this->assertContains('modules/system/tests/fixtures/themes/assettest', $packageJson['workspaces']['packages']); + }); + } + + public function testMixInstallMultiplePackages(): void + { + $this->withPackageJsonRestore(function () { + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest', 'theme-npmtest'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->expectsOutput('Adding theme-npmtest (modules/system/tests/fixtures/themes/npmtest) to the workspaces.packages property in package.json') + ->assertExitCode(0); + + $this->assertFileExists($this->jsonPath); + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('laravel-mix', $packageJson['devDependencies']); + + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayHasKey('packages', $packageJson['workspaces']); + $this->assertContains('modules/system/tests/fixtures/themes/assettest', $packageJson['workspaces']['packages']); + $this->assertContains('modules/system/tests/fixtures/themes/npmtest', $packageJson['workspaces']['packages']); + }); + } + + public function testMixInstallMissingPackage(): void + { + // We should receive an exception for a missing package + $this->expectException(PackageNotFoundException::class); + + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest2'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->assertExitCode(1); + } + + public function testMixInstallRelativePath(): void + { + $this->withPackageJsonRestore(function () { + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => 'modules/system/tests/package.json', + '--no-install' => true + ]) + ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->assertExitCode(0); + }); + } + + public function testMixInstallIgnoredPackage(): void + { + $this->withPackageJsonRestore(function () { + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $packageJson['workspaces'] = [ + 'ignoredPackages' => [ + 'modules/system/tests/fixtures/themes/assettest' + ] + ]; + File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); + + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', false) + ->expectsOutput('The requested package theme-assettest (modules/system/tests/fixtures/themes/assettest) is ignored, remove it from package.json to continue.') + ->assertExitCode(0); + + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayNotHasKey('packages', $packageJson['workspaces']); + $this->assertArrayNotHasKey('dependencies', $packageJson); + $this->assertArrayNotHasKey('devDependencies', $packageJson); + }); + } + + public function testMixInstallWithNpmInstall(): void + { + $this->withPackageJsonRestore(function () { + $this->assertDirectoryDoesNotExist($this->fixturePath . '/node_modules'); + + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => $this->jsonPath, + '--disable-tty' => true + ]) + ->expectsQuestion('laravel-mix was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->expectsOutputToContain('packages, and audited') // output from npm i + ->assertExitCode(0); + + $this->assertFileExists($this->jsonPath); + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('laravel-mix', $packageJson['devDependencies']); + + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayHasKey('packages', $packageJson['workspaces']); + $this->assertContains('modules/system/tests/fixtures/themes/assettest', $packageJson['workspaces']['packages']); + + $this->assertFileExists($this->lockPath); + + $this->assertDirectoryExists($this->fixturePath . '/node_modules'); + $this->assertDirectoryExists($this->fixturePath . '/node_modules/laravel-mix'); + }); + } + + public function tearDown(): void + { + if (File::isDirectory($this->fixturePath . '/node_modules')) { + File::deleteDirectory($this->fixturePath . '/node_modules'); + } + + if (File::exists($this->lockPath)) { + File::delete($this->lockPath); + } + + parent::tearDown(); + } +} diff --git a/modules/system/tests/console/asset/vite/ViteInstallTest.php b/modules/system/tests/console/asset/vite/ViteInstallTest.php new file mode 100644 index 0000000000..c6b471aac4 --- /dev/null +++ b/modules/system/tests/console/asset/vite/ViteInstallTest.php @@ -0,0 +1,201 @@ +markTestSkipped('This test requires node_modules to be installed'); + } + + // Define some helpful paths + $this->fixturePath = base_path('modules/system/tests'); + $this->jsonPath = $this->fixturePath . '/package.json'; + $this->lockPath = $this->fixturePath . '/package-lock.json'; + $this->backupPath = $this->fixturePath . '/package.backup.json'; + + // Add our testing theme because it won't be auto discovered + PackageManager::instance()->registerPackage( + 'theme-assettest', + base_path('modules/system/tests/fixtures/themes/assettest/vite.config.mjs'), + 'vite' + ); + PackageManager::instance()->registerPackage( + 'theme-npmtest', + base_path('modules/system/tests/fixtures/themes/npmtest/vite.config.mjs'), + 'vite' + ); + } + + public function testViteInstallSinglePackage(): void + { + $this->withPackageJsonRestore(function () { + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) + ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->assertExitCode(0); + + $this->assertFileExists($this->jsonPath); + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('vite', $packageJson['devDependencies']); + $this->assertArrayHasKey('laravel-vite-plugin', $packageJson['devDependencies']); + + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayHasKey('packages', $packageJson['workspaces']); + $this->assertContains('modules/system/tests/fixtures/themes/assettest', $packageJson['workspaces']['packages']); + }); + } + + public function testViteInstallMultiplePackages(): void + { + $this->withPackageJsonRestore(function () { + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest', 'theme-npmtest'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) + ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->expectsOutput('Adding theme-npmtest (modules/system/tests/fixtures/themes/npmtest) to the workspaces.packages property in package.json') + ->assertExitCode(0); + + $this->assertFileExists($this->jsonPath); + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('vite', $packageJson['devDependencies']); + $this->assertArrayHasKey('laravel-vite-plugin', $packageJson['devDependencies']); + + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayHasKey('packages', $packageJson['workspaces']); + $this->assertContains('modules/system/tests/fixtures/themes/assettest', $packageJson['workspaces']['packages']); + $this->assertContains('modules/system/tests/fixtures/themes/npmtest', $packageJson['workspaces']['packages']); + }); + } + + public function testViteInstallMissingPackage(): void + { + // We should receive an exception for a missing package + $this->expectException(PackageNotFoundException::class); + + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest2'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->assertExitCode(1); + } + + public function testViteInstallRelativePath(): void + { + $this->withPackageJsonRestore(function () { + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => 'modules/system/tests/package.json', + '--no-install' => true + ]) + ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) + ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->assertExitCode(0); + }); + } + + public function testViteInstallIgnoredPackage(): void + { + $this->withPackageJsonRestore(function () { + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $packageJson['workspaces'] = [ + 'ignoredPackages' => [ + 'modules/system/tests/fixtures/themes/assettest' + ] + ]; + File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); + + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', false) + ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', false) + ->expectsOutput('The requested package theme-assettest (modules/system/tests/fixtures/themes/assettest) is ignored, remove it from package.json to continue.') + ->assertExitCode(0); + + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayNotHasKey('packages', $packageJson['workspaces']); + $this->assertArrayNotHasKey('dependencies', $packageJson); + $this->assertArrayNotHasKey('devDependencies', $packageJson); + }); + } + + public function testViteInstallWithNpmInstall(): void + { + $this->withPackageJsonRestore(function () { + $this->assertDirectoryDoesNotExist($this->fixturePath . '/node_modules'); + + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => $this->jsonPath, + '--disable-tty' => true + ]) + ->expectsQuestion('vite was not found as a dependency in package.json, would you like to add it?', true) + ->expectsQuestion('laravel-vite-plugin was not found as a dependency in package.json, would you like to add it?', true) + ->expectsOutput('Adding theme-assettest (modules/system/tests/fixtures/themes/assettest) to the workspaces.packages property in package.json') + ->expectsOutputToContain('packages, and audited') // output from npm i + ->assertExitCode(0); + + $this->assertFileExists($this->jsonPath); + $packageJson = json_decode(File::get($this->jsonPath), JSON_OBJECT_AS_ARRAY); + $this->assertArrayHasKey('devDependencies', $packageJson); + $this->assertArrayHasKey('vite', $packageJson['devDependencies']); + $this->assertArrayHasKey('laravel-vite-plugin', $packageJson['devDependencies']); + + $this->assertArrayHasKey('workspaces', $packageJson); + $this->assertArrayHasKey('packages', $packageJson['workspaces']); + $this->assertContains('modules/system/tests/fixtures/themes/assettest', $packageJson['workspaces']['packages']); + + $this->assertFileExists($this->lockPath); + + $this->assertDirectoryExists($this->fixturePath . '/node_modules'); + $this->assertDirectoryExists($this->fixturePath . '/node_modules/vite'); + $this->assertDirectoryExists($this->fixturePath . '/node_modules/laravel-vite-plugin'); + }); + } + + public function tearDown(): void + { + if (File::isDirectory($this->fixturePath . '/node_modules')) { + File::deleteDirectory($this->fixturePath . '/node_modules'); + } + + if (File::exists($this->lockPath)) { + File::delete($this->lockPath); + } + + parent::tearDown(); + } +} From 2a222bf9a016a28cab7a2d43a5c8c673b3f839e9 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 18:01:36 +0100 Subject: [PATCH 18/28] Removed json_validate as it's only available in php8.3 --- modules/system/classes/asset/PackageJson.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/modules/system/classes/asset/PackageJson.php b/modules/system/classes/asset/PackageJson.php index 7e4900993c..cf88c51656 100644 --- a/modules/system/classes/asset/PackageJson.php +++ b/modules/system/classes/asset/PackageJson.php @@ -28,11 +28,10 @@ public function __construct( protected ?string $path = null ) { if (File::exists($this->path)) { - $raw = File::get($this->path); - if (!json_validate($raw)) { + $this->data = json_decode(File::get($this->path), JSON_OBJECT_AS_ARRAY); + if (json_last_error() !== JSON_ERROR_NONE) { throw new \JsonException('The contents of the file "' . $this->path . '" is not valid json.'); } - $this->data = json_decode($raw, JSON_OBJECT_AS_ARRAY); } } From a8b119fd34008daa15625188458db674430622f9 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Mon, 23 Sep 2024 18:05:20 +0100 Subject: [PATCH 19/28] Updated eslintignore --- modules/system/.eslintignore | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/modules/system/.eslintignore b/modules/system/.eslintignore index cc2943407d..20eac9f232 100644 --- a/modules/system/.eslintignore +++ b/modules/system/.eslintignore @@ -13,4 +13,6 @@ assets/vendor # Ignore test fixtures tests/js tests/fixtures/themes/test/assets/js -tests/fixtures/themes/assettest/assets/javascript +tests/fixtures/themes/npmtest +tests/fixtures/themes/assettest +tests/fixtures/plugins/mix From 5b49911919d229443f8f353335ef4ce660dfbcc0 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Tue, 24 Sep 2024 15:08:23 +0100 Subject: [PATCH 20/28] Added SystemException as base for package exceptions --- .../console/asset/exceptions/PackageIgnoredException.php | 4 +++- .../asset/exceptions/PackageNotConfiguredException.php | 4 +++- .../console/asset/exceptions/PackageNotFoundException.php | 4 +++- .../asset/exceptions/PackageNotRegisteredException.php | 4 +++- 4 files changed, 12 insertions(+), 4 deletions(-) diff --git a/modules/system/console/asset/exceptions/PackageIgnoredException.php b/modules/system/console/asset/exceptions/PackageIgnoredException.php index c81d60db0d..b67ff122e5 100644 --- a/modules/system/console/asset/exceptions/PackageIgnoredException.php +++ b/modules/system/console/asset/exceptions/PackageIgnoredException.php @@ -2,7 +2,9 @@ namespace System\Console\Asset\Exceptions; -class PackageIgnoredException extends \InvalidArgumentException +use Winter\Storm\Exception\SystemException; + +class PackageIgnoredException extends SystemException { } diff --git a/modules/system/console/asset/exceptions/PackageNotConfiguredException.php b/modules/system/console/asset/exceptions/PackageNotConfiguredException.php index 21a692e3df..74e47f4adc 100644 --- a/modules/system/console/asset/exceptions/PackageNotConfiguredException.php +++ b/modules/system/console/asset/exceptions/PackageNotConfiguredException.php @@ -2,7 +2,9 @@ namespace System\Console\Asset\Exceptions; -class PackageNotConfiguredException extends \InvalidArgumentException +use Winter\Storm\Exception\SystemException; + +class PackageNotConfiguredException extends SystemException { } diff --git a/modules/system/console/asset/exceptions/PackageNotFoundException.php b/modules/system/console/asset/exceptions/PackageNotFoundException.php index 62b284d346..7aa0378a5e 100644 --- a/modules/system/console/asset/exceptions/PackageNotFoundException.php +++ b/modules/system/console/asset/exceptions/PackageNotFoundException.php @@ -2,7 +2,9 @@ namespace System\Console\Asset\Exceptions; -class PackageNotFoundException extends \InvalidArgumentException +use Winter\Storm\Exception\SystemException; + +class PackageNotFoundException extends SystemException { } diff --git a/modules/system/console/asset/exceptions/PackageNotRegisteredException.php b/modules/system/console/asset/exceptions/PackageNotRegisteredException.php index 0079e56162..6c7576c1c5 100644 --- a/modules/system/console/asset/exceptions/PackageNotRegisteredException.php +++ b/modules/system/console/asset/exceptions/PackageNotRegisteredException.php @@ -2,7 +2,9 @@ namespace System\Console\Asset\Exceptions; -class PackageNotRegisteredException extends \InvalidArgumentException +use Winter\Storm\Exception\SystemException; + +class PackageNotRegisteredException extends SystemException { } From dc28b28454d1629f8ce1c1c602c1a7b5f56738b5 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Tue, 24 Sep 2024 15:10:06 +0100 Subject: [PATCH 21/28] Added corrupt test for PackageJson --- modules/system/classes/asset/PackageJson.php | 6 +++++- modules/system/tests/classes/asset/PackageJsonTest.php | 9 +++++++++ modules/system/tests/fixtures/npm/package-corrupt.json | 8 ++++++++ 3 files changed, 22 insertions(+), 1 deletion(-) create mode 100644 modules/system/tests/fixtures/npm/package-corrupt.json diff --git a/modules/system/classes/asset/PackageJson.php b/modules/system/classes/asset/PackageJson.php index cf88c51656..ef115b7724 100644 --- a/modules/system/classes/asset/PackageJson.php +++ b/modules/system/classes/asset/PackageJson.php @@ -23,15 +23,19 @@ class PackageJson /** * Create a new instance with optional path, loads file if file already exists + * @throws \JsonException */ public function __construct( protected ?string $path = null ) { if (File::exists($this->path)) { - $this->data = json_decode(File::get($this->path), JSON_OBJECT_AS_ARRAY); + // Test the json to insure it's valid + $json = json_decode(File::get($this->path), JSON_OBJECT_AS_ARRAY); if (json_last_error() !== JSON_ERROR_NONE) { throw new \JsonException('The contents of the file "' . $this->path . '" is not valid json.'); } + + $this->data = $json; } } diff --git a/modules/system/tests/classes/asset/PackageJsonTest.php b/modules/system/tests/classes/asset/PackageJsonTest.php index ed8d244e9b..e7b1ebdcd1 100644 --- a/modules/system/tests/classes/asset/PackageJsonTest.php +++ b/modules/system/tests/classes/asset/PackageJsonTest.php @@ -34,6 +34,15 @@ public function testLoadFile(): void $this->assertIsArray($contents['workspaces']['packages']); } + /** + * Test loading a corrupted package.json file correctly errors + */ + public function testLoadCorruptFile(): void + { + $this->expectException(\JsonException::class); + new PackageJson(__DIR__ . '/../../fixtures/npm/package-corrupt.json'); + } + /** * Test creating an instance with non-existing file */ diff --git a/modules/system/tests/fixtures/npm/package-corrupt.json b/modules/system/tests/fixtures/npm/package-corrupt.json new file mode 100644 index 0000000000..35f6291a84 --- /dev/null +++ b/modules/system/tests/fixtures/npm/package-corrupt.json @@ -0,0 +1,8 @@ +{ + "workspaces": { + "packages": [ + "plugins/winter/demo", + "themes/demo" + ] + }, + "dependencies": From 54843a6dfa14e5c36d8feaa10bf7f97a44dff1ac Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Tue, 24 Sep 2024 15:11:08 +0100 Subject: [PATCH 22/28] Added check for PackageManager to ensure the config file for a registered package exists --- modules/system/classes/asset/PackageManager.php | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/modules/system/classes/asset/PackageManager.php b/modules/system/classes/asset/PackageManager.php index 231cc55baa..725b86bd92 100644 --- a/modules/system/classes/asset/PackageManager.php +++ b/modules/system/classes/asset/PackageManager.php @@ -315,6 +315,17 @@ public function registerPackage(string $name, string $path, string $type = 'mix' )); } + $package = $path . '/package.json'; + $config = $path . DIRECTORY_SEPARATOR . $configFile; + + if (!File::exists($config)) { + throw new SystemException(sprintf( + 'Cannot register "%s" as a compilable package; the config file "%s" does not exist.', + $name, + $config + )); + } + // Check for any existing packages already registered under the provided name if (isset($this->packages[$name])) { throw new SystemException(sprintf( @@ -324,9 +335,6 @@ public function registerPackage(string $name, string $path, string $type = 'mix' )); } - $package = $path . '/package.json'; - $config = $path . DIRECTORY_SEPARATOR . $configFile; - // Check for any existing package that already registers the given compilable config path foreach ($this->packages[$type] ?? [] as $packageName => $settings) { if ($settings['config'] === $config) { From 1969a0dc484c7d30ca0bad7cddb31ae2f1b6963c Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Tue, 24 Sep 2024 16:13:15 +0100 Subject: [PATCH 23/28] Renamed test package.json file --- modules/system/tests/{package.json => package-testing.json} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename modules/system/tests/{package.json => package-testing.json} (100%) diff --git a/modules/system/tests/package.json b/modules/system/tests/package-testing.json similarity index 100% rename from modules/system/tests/package.json rename to modules/system/tests/package-testing.json From 7c64366db2577be9ed50ef96fb7eac9c56f70d41 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Tue, 24 Sep 2024 16:15:22 +0100 Subject: [PATCH 24/28] Added fix for package json restoring and added test case for --package-json --- .../console/asset/mix/MixInstallTest.php | 44 ++++++++++++++----- .../console/asset/vite/ViteInstallTest.php | 42 +++++++++++++----- 2 files changed, 63 insertions(+), 23 deletions(-) diff --git a/modules/system/tests/console/asset/mix/MixInstallTest.php b/modules/system/tests/console/asset/mix/MixInstallTest.php index 332fcf64bc..1494610628 100644 --- a/modules/system/tests/console/asset/mix/MixInstallTest.php +++ b/modules/system/tests/console/asset/mix/MixInstallTest.php @@ -3,15 +3,12 @@ namespace System\Tests\Console\Asset\Mix; use System\Classes\Asset\PackageManager; -use System\Console\Asset\Exceptions\PackageNotFoundException; use System\Tests\Bootstrap\TestCase; -use System\Tests\Console\Asset\NpmTestTrait; +use Winter\Storm\Exception\SystemException; use Winter\Storm\Support\Facades\File; class MixInstallTest extends TestCase { - use NpmTestTrait; - protected string $fixturePath; protected string $jsonPath; protected string $lockPath; @@ -29,7 +26,7 @@ public function setUp(): void $this->fixturePath = base_path('modules/system/tests'); $this->jsonPath = $this->fixturePath . '/package.json'; $this->lockPath = $this->fixturePath . '/package-lock.json'; - $this->backupPath = $this->fixturePath . '/package.backup.json'; + $this->backupPath = $this->fixturePath . '/package-testing.json'; // Add our testing theme because it won't be auto discovered PackageManager::instance()->registerPackage( @@ -44,6 +41,17 @@ public function setUp(): void ); } + public function testMixInstallMissingPackageJson(): void + { + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => '/some/file', + '--no-install' => true + ]) + ->expectsOutputToContain('The supplied --package-json path does not exist.') + ->assertExitCode(1); + } + public function testMixInstallSinglePackage(): void { $this->withPackageJsonRestore(function () { @@ -95,14 +103,16 @@ public function testMixInstallMultiplePackages(): void public function testMixInstallMissingPackage(): void { // We should receive an exception for a missing package - $this->expectException(PackageNotFoundException::class); + $this->withPackageJsonRestore(function () { + $this->expectException(SystemException::class); - $this->artisan('mix:install', [ - 'assetPackage' => ['theme-assettest2'], - '--package-json' => $this->jsonPath, - '--no-install' => true - ]) - ->assertExitCode(1); + $this->artisan('mix:install', [ + 'assetPackage' => ['theme-assettest2'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->assertExitCode(1); + }); } public function testMixInstallRelativePath(): void @@ -178,6 +188,16 @@ public function testMixInstallWithNpmInstall(): void }); } + /** + * Helper to run test logic and handle restoring package.json file after + */ + protected function withPackageJsonRestore(callable $callback): void + { + File::copy($this->backupPath, $this->jsonPath); + $callback(); + File::delete($this->jsonPath); + } + public function tearDown(): void { if (File::isDirectory($this->fixturePath . '/node_modules')) { diff --git a/modules/system/tests/console/asset/vite/ViteInstallTest.php b/modules/system/tests/console/asset/vite/ViteInstallTest.php index c6b471aac4..c88abafc11 100644 --- a/modules/system/tests/console/asset/vite/ViteInstallTest.php +++ b/modules/system/tests/console/asset/vite/ViteInstallTest.php @@ -5,13 +5,10 @@ use System\Classes\Asset\PackageManager; use System\Console\Asset\Exceptions\PackageNotFoundException; use System\Tests\Bootstrap\TestCase; -use System\Tests\Console\Asset\NpmTestTrait; use Winter\Storm\Support\Facades\File; class ViteInstallTest extends TestCase { - use NpmTestTrait; - protected string $jsonPath; protected string $lockPath; protected string $backupPath; @@ -28,7 +25,7 @@ public function setUp(): void $this->fixturePath = base_path('modules/system/tests'); $this->jsonPath = $this->fixturePath . '/package.json'; $this->lockPath = $this->fixturePath . '/package-lock.json'; - $this->backupPath = $this->fixturePath . '/package.backup.json'; + $this->backupPath = $this->fixturePath . '/package-testing.json'; // Add our testing theme because it won't be auto discovered PackageManager::instance()->registerPackage( @@ -43,6 +40,17 @@ public function setUp(): void ); } + public function testViteInstallMissingPackageJson(): void + { + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest'], + '--package-json' => '/some/file', + '--no-install' => true + ]) + ->expectsOutputToContain('The supplied --package-json path does not exist.') + ->assertExitCode(1); + } + public function testViteInstallSinglePackage(): void { $this->withPackageJsonRestore(function () { @@ -98,14 +106,16 @@ public function testViteInstallMultiplePackages(): void public function testViteInstallMissingPackage(): void { // We should receive an exception for a missing package - $this->expectException(PackageNotFoundException::class); + $this->withPackageJsonRestore(function () { + $this->expectException(PackageNotFoundException::class); - $this->artisan('vite:install', [ - 'assetPackage' => ['theme-assettest2'], - '--package-json' => $this->jsonPath, - '--no-install' => true - ]) - ->assertExitCode(1); + $this->artisan('vite:install', [ + 'assetPackage' => ['theme-assettest2'], + '--package-json' => $this->jsonPath, + '--no-install' => true + ]) + ->assertExitCode(1); + }); } public function testViteInstallRelativePath(): void @@ -186,6 +196,16 @@ public function testViteInstallWithNpmInstall(): void }); } + /** + * Helper to run test logic and handle restoring package.json file after + */ + protected function withPackageJsonRestore(callable $callback): void + { + File::copy($this->backupPath, $this->jsonPath); + $callback(); + File::delete($this->jsonPath); + } + public function tearDown(): void { if (File::isDirectory($this->fixturePath . '/node_modules')) { From 76fec292ff13c541b54e0deae32cba89e2207aa7 Mon Sep 17 00:00:00 2001 From: Luke Towers Date: Tue, 24 Sep 2024 11:09:23 -0600 Subject: [PATCH 25/28] Update modules/system/console/asset/npm/NpmInstall.php --- modules/system/console/asset/npm/NpmInstall.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/system/console/asset/npm/NpmInstall.php b/modules/system/console/asset/npm/NpmInstall.php index b3b0526cdc..f77fc29e5a 100644 --- a/modules/system/console/asset/npm/NpmInstall.php +++ b/modules/system/console/asset/npm/NpmInstall.php @@ -1,6 +1,6 @@ Date: Tue, 24 Sep 2024 11:09:48 -0600 Subject: [PATCH 26/28] Update modules/system/console/asset/AssetInstall.php --- modules/system/console/asset/AssetInstall.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/system/console/asset/AssetInstall.php b/modules/system/console/asset/AssetInstall.php index 901299d632..af3c650473 100644 --- a/modules/system/console/asset/AssetInstall.php +++ b/modules/system/console/asset/AssetInstall.php @@ -9,7 +9,7 @@ use System\Classes\Asset\PackageManager; use System\Classes\PluginManager; use System\Console\Asset\Exceptions\PackageIgnoredException; -use System\console\asset\exceptions\PackageNotConfiguredException; +use System\Console\Asset\Exceptions\PackageNotConfiguredException; use System\Console\Asset\Exceptions\PackageNotFoundException; use Winter\Storm\Console\Command; use Winter\Storm\Support\Facades\Config; From c39bdb2afd2e85a8e577788fd98b2ff94014d52f Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Fri, 4 Oct 2024 10:05:52 +0100 Subject: [PATCH 27/28] Added phpdocs --- .../system/classes/asset/PackageManager.php | 6 ++++++ modules/system/console/asset/AssetInstall.php | 21 +++++++++++++++++++ .../console/asset/{ => npm}/NpmCommand.php | 13 +++++++++++- .../system/console/asset/npm/NpmInstall.php | 7 +++++-- modules/system/console/asset/npm/NpmRun.php | 2 +- .../system/console/asset/npm/NpmUpdate.php | 5 ++++- 6 files changed, 49 insertions(+), 5 deletions(-) rename modules/system/console/asset/{ => npm}/NpmCommand.php (86%) diff --git a/modules/system/classes/asset/PackageManager.php b/modules/system/classes/asset/PackageManager.php index 725b86bd92..bf84d147d6 100644 --- a/modules/system/classes/asset/PackageManager.php +++ b/modules/system/classes/asset/PackageManager.php @@ -355,6 +355,9 @@ public function registerPackage(string $name, string $path, string $type = 'mix' ]; } + /** + * Returns an expected package type from its name + */ public function getPackageTypeFromName(string $package): ?string { // Check if package could be a module @@ -379,6 +382,9 @@ public function getPackageTypeFromName(string $package): ?string return null; } + /** + * Set the package.json file path used for checking if packages are in workspaces or ignored + */ public function setPackageJsonPath(string $packageJsonPath): static { $this->packageJson = new PackageJson($packageJsonPath); diff --git a/modules/system/console/asset/AssetInstall.php b/modules/system/console/asset/AssetInstall.php index 901299d632..41d4d78485 100644 --- a/modules/system/console/asset/AssetInstall.php +++ b/modules/system/console/asset/AssetInstall.php @@ -125,6 +125,11 @@ public function handle(): int return 0; } + /** + * Returns all packages registered by the system filtered by requestedPackages if defined + * @throws PackageNotFoundException + * @throws \Winter\Storm\Exception\SystemException + */ protected function getRegisteredPackages(array $requestedPackages = []): array { $packageManager = $this->getPackageManager(); @@ -193,6 +198,10 @@ protected function getRegisteredPackages(array $requestedPackages = []): array return $registeredPackages; } + /** + * Checks if the package.json of a package has the dependencies required for this command and asks the user if + * they want to install them if not present. + */ protected function validateRequireDependenciesPresent(PackageJson $packageJson): PackageJson { // Check to see if required packages are already present as a dependency @@ -208,6 +217,12 @@ protected function validateRequireDependenciesPresent(PackageJson $packageJson): return $packageJson; } + /** + * Validates if the packages passed can be installed and if possible, installs them. + * @throws PackageIgnoredException + * @throws PackageNotConfiguredException + * @throws PackageNotFoundException + */ protected function processPackages(array $registeredPackages, PackageJson $packageJson): PackageJson { // Check if the user requested a specific package for install @@ -267,6 +282,9 @@ protected function processPackages(array $registeredPackages, PackageJson $packa return $packageJson; } + /** + * Adds a package to the project workspace or mark it as ignored based on user input + */ protected function processPackage(PackageJson $packageJson, string $name, array $package, bool $force = false): bool { // Normalize package path across OS types @@ -366,6 +384,9 @@ protected function runNpmInstall(): int } } + /** + * Returns the root package.json as a PackageManager object + */ protected function getPackageManager(): PackageManager { // Flush the instance diff --git a/modules/system/console/asset/NpmCommand.php b/modules/system/console/asset/npm/NpmCommand.php similarity index 86% rename from modules/system/console/asset/NpmCommand.php rename to modules/system/console/asset/npm/NpmCommand.php index 220a42befd..d90bdb012d 100644 --- a/modules/system/console/asset/NpmCommand.php +++ b/modules/system/console/asset/npm/NpmCommand.php @@ -1,6 +1,6 @@ hasOption('production')) { diff --git a/modules/system/console/asset/npm/NpmInstall.php b/modules/system/console/asset/npm/NpmInstall.php index b3b0526cdc..eb7d75d476 100644 --- a/modules/system/console/asset/npm/NpmInstall.php +++ b/modules/system/console/asset/npm/NpmInstall.php @@ -1,9 +1,9 @@ argument('npmArgs')) ?? []; diff --git a/modules/system/console/asset/npm/NpmRun.php b/modules/system/console/asset/npm/NpmRun.php index ee4a7d7457..cd45313acd 100644 --- a/modules/system/console/asset/npm/NpmRun.php +++ b/modules/system/console/asset/npm/NpmRun.php @@ -2,7 +2,7 @@ namespace System\Console\Asset\Npm; -use System\Console\Asset\NpmCommand; +use System\Console\Asset\Npm\NpmCommand; class NpmRun extends NpmCommand { diff --git a/modules/system/console/asset/npm/NpmUpdate.php b/modules/system/console/asset/npm/NpmUpdate.php index 144a991da3..ad818200a2 100644 --- a/modules/system/console/asset/npm/NpmUpdate.php +++ b/modules/system/console/asset/npm/NpmUpdate.php @@ -3,7 +3,7 @@ namespace System\Console\Asset\Npm; use System\Console\Asset\Exceptions\PackageNotRegisteredException; -use System\Console\Asset\NpmCommand; +use System\Console\Asset\Npm\NpmCommand; class NpmUpdate extends NpmCommand { @@ -35,6 +35,9 @@ class NpmUpdate extends NpmCommand 'mix:update' ]; + /** + * Execute the console command. + */ public function handle(): int { $command = ($this->argument('npmArgs')) ?? []; From 2f16357bcdb227532676985353cbc787d84b3508 Mon Sep 17 00:00:00 2001 From: Jack Wilkinson Date: Fri, 25 Oct 2024 11:22:13 +0100 Subject: [PATCH 28/28] Removed custom exceptions --- modules/system/console/asset/AssetInstall.php | 30 ++++++++----------- .../exceptions/PackageIgnoredException.php | 10 ------- .../PackageNotConfiguredException.php | 10 ------- .../exceptions/PackageNotFoundException.php | 10 ------- .../PackageNotRegisteredException.php | 10 ------- .../system/console/asset/npm/NpmCommand.php | 6 ++-- .../system/console/asset/npm/NpmInstall.php | 7 +++-- .../system/console/asset/npm/NpmUpdate.php | 7 +++-- .../console/asset/vite/ViteInstallTest.php | 6 ++-- 9 files changed, 30 insertions(+), 66 deletions(-) delete mode 100644 modules/system/console/asset/exceptions/PackageIgnoredException.php delete mode 100644 modules/system/console/asset/exceptions/PackageNotConfiguredException.php delete mode 100644 modules/system/console/asset/exceptions/PackageNotFoundException.php delete mode 100644 modules/system/console/asset/exceptions/PackageNotRegisteredException.php diff --git a/modules/system/console/asset/AssetInstall.php b/modules/system/console/asset/AssetInstall.php index 38c5c093bc..fcd9013c66 100644 --- a/modules/system/console/asset/AssetInstall.php +++ b/modules/system/console/asset/AssetInstall.php @@ -8,10 +8,8 @@ use System\Classes\Asset\PackageJson; use System\Classes\Asset\PackageManager; use System\Classes\PluginManager; -use System\Console\Asset\Exceptions\PackageIgnoredException; -use System\Console\Asset\Exceptions\PackageNotConfiguredException; -use System\Console\Asset\Exceptions\PackageNotFoundException; use Winter\Storm\Console\Command; +use Winter\Storm\Exception\SystemException; use Winter\Storm\Support\Facades\Config; use Winter\Storm\Support\Facades\File; use Winter\Storm\Support\Str; @@ -127,8 +125,7 @@ public function handle(): int /** * Returns all packages registered by the system filtered by requestedPackages if defined - * @throws PackageNotFoundException - * @throws \Winter\Storm\Exception\SystemException + * @throws SystemException */ protected function getRegisteredPackages(array $requestedPackages = []): array { @@ -177,8 +174,8 @@ protected function getRegisteredPackages(array $requestedPackages = []): array ); break; case null: - throw new PackageNotFoundException(sprintf( - 'The package `%s` does not exist.', + throw new SystemException(sprintf( + 'PackageNotFoundException: The package `%s` does not exist.', $package )); } @@ -219,8 +216,7 @@ protected function validateRequireDependenciesPresent(PackageJson $packageJson): /** * Validates if the packages passed can be installed and if possible, installs them. - * @throws PackageIgnoredException - * @throws PackageNotConfiguredException + * @throws SystemException * @throws PackageNotFoundException */ protected function processPackages(array $registeredPackages, PackageJson $packageJson): PackageJson @@ -235,8 +231,8 @@ protected function processPackages(array $registeredPackages, PackageJson $packa switch (count($detected)) { case 1: if ($detected[0]['type'] !== $this->assetType) { - throw new PackageNotConfiguredException(sprintf( - 'The requested package `%s` is only configured for %s. Run `php artisan %s:create %1$s`', + throw new SystemException(sprintf( + 'PackageNotConfiguredException: The requested package `%s` is only configured for %s. Run `php artisan %s:create %1$s`', $requestedPackage, $detected[0]['type'], $this->assetType @@ -244,8 +240,8 @@ protected function processPackages(array $registeredPackages, PackageJson $packa } if ($detected[0]['ignored']) { - throw new PackageIgnoredException(sprintf( - 'The requested package `%s` is ignored, remove it from package.json to continue', + throw new SystemException(sprintf( + 'PackageIgnoredException: The requested package `%s` is ignored, remove it from package.json to continue', $requestedPackage, )); } @@ -253,8 +249,8 @@ protected function processPackages(array $registeredPackages, PackageJson $packa case 2: default: if (($detected[0]['ignored'] ?? false) || ($detected[1]['ignored'] ?? false)) { - throw new PackageIgnoredException(sprintf( - 'The requested package `%s` is ignored, remove it from package.json to continue', + throw new SystemException(sprintf( + 'PackageIgnoredException: The requested package `%s` is ignored, remove it from package.json to continue', $requestedPackage, )); } @@ -262,8 +258,8 @@ protected function processPackages(array $registeredPackages, PackageJson $packa } } - throw new PackageNotFoundException(sprintf( - 'The requested package `%s` could not be found.', + throw new SystemException(sprintf( + 'PackageNotFoundException: The requested package `%s` could not be found.', $requestedPackage, )); } diff --git a/modules/system/console/asset/exceptions/PackageIgnoredException.php b/modules/system/console/asset/exceptions/PackageIgnoredException.php deleted file mode 100644 index b67ff122e5..0000000000 --- a/modules/system/console/asset/exceptions/PackageIgnoredException.php +++ /dev/null @@ -1,10 +0,0 @@ -hasPackage($name, true)) { - throw new PackageNotRegisteredException(sprintf('Package "%s" is not a registered package.', $name)); + throw new SystemException(sprintf('Package "%s" is not a registered package.', $name)); } $package = $compilableAssets->getPackage($name, true)[0] ?? []; diff --git a/modules/system/console/asset/npm/NpmInstall.php b/modules/system/console/asset/npm/NpmInstall.php index eb7d75d476..da697e42ae 100644 --- a/modules/system/console/asset/npm/NpmInstall.php +++ b/modules/system/console/asset/npm/NpmInstall.php @@ -2,8 +2,8 @@ namespace System\Console\Asset\Npm; -use System\Console\Asset\Exceptions\PackageNotRegisteredException; use System\Console\Asset\Npm\NpmCommand; +use Winter\Storm\Exception\SystemException; class NpmInstall extends NpmCommand { @@ -37,7 +37,10 @@ public function handle(): int try { [$package, $packageJson] = $this->getPackage(); - } catch (PackageNotRegisteredException $e) { + } catch (SystemException $e) { + if (!str_contains($e->getMessage(), 'is not a registered package.')) { + throw $e; + } array_unshift($command, $this->argument('package')); } diff --git a/modules/system/console/asset/npm/NpmUpdate.php b/modules/system/console/asset/npm/NpmUpdate.php index ad818200a2..f578c5c4c1 100644 --- a/modules/system/console/asset/npm/NpmUpdate.php +++ b/modules/system/console/asset/npm/NpmUpdate.php @@ -2,8 +2,8 @@ namespace System\Console\Asset\Npm; -use System\Console\Asset\Exceptions\PackageNotRegisteredException; use System\Console\Asset\Npm\NpmCommand; +use Winter\Storm\Exception\SystemException; class NpmUpdate extends NpmCommand { @@ -44,7 +44,10 @@ public function handle(): int try { [$package, $packageJson] = $this->getPackage(); - } catch (PackageNotRegisteredException $e) { + } catch (SystemException $e) { + if (!str_contains($e->getMessage(), 'is not a registered package.')) { + throw $e; + } array_unshift($command, $this->argument('package')); } diff --git a/modules/system/tests/console/asset/vite/ViteInstallTest.php b/modules/system/tests/console/asset/vite/ViteInstallTest.php index c88abafc11..7d97ccf107 100644 --- a/modules/system/tests/console/asset/vite/ViteInstallTest.php +++ b/modules/system/tests/console/asset/vite/ViteInstallTest.php @@ -3,8 +3,8 @@ namespace System\Tests\Console\Asset\Vite; use System\Classes\Asset\PackageManager; -use System\Console\Asset\Exceptions\PackageNotFoundException; use System\Tests\Bootstrap\TestCase; +use Winter\Storm\Exception\SystemException; use Winter\Storm\Support\Facades\File; class ViteInstallTest extends TestCase @@ -107,7 +107,8 @@ public function testViteInstallMissingPackage(): void { // We should receive an exception for a missing package $this->withPackageJsonRestore(function () { - $this->expectException(PackageNotFoundException::class); + $this->expectException(SystemException::class); + $this->expectExceptionMessage('PackageNotFoundException: The package `theme-assettest2` does not exist.'); $this->artisan('vite:install', [ 'assetPackage' => ['theme-assettest2'], @@ -142,6 +143,7 @@ public function testViteInstallIgnoredPackage(): void 'modules/system/tests/fixtures/themes/assettest' ] ]; + File::put($this->jsonPath, json_encode($packageJson, JSON_PRETTY_PRINT)); $this->artisan('vite:install', [