bug symfony#996 AssetMapper: upgrade packages when needed (weaverryan)

This PR was squashed before being merged into the 1.x branch. Discussion ---------- AssetMapper: upgrade packages when needed Hi! The scenario: * User installs `symfony/ux-autocomplete`. Flex adds `tom-select` at version `2.2.3` to `importmap.php` * 3 months later, user upgrades `symfony/ux-autocomplete`. The new version now requires `tom-select` at `^2.5`. If we do nothing, the user's `tom-select` is out of date and the user won't even know about it. The `^2.5` constraint is defined in the `symfony/ux-autocomplete` [package.json file](https://github.com/symfony/ux/blob/97fe2cd62e70e329b85570e0b797833882930754/src/Autocomplete/assets/package.json#L23), but nothing enforces that or notifies the user. With this small change, during a composer update/require, we look at the dependencies of all UX packages and compare them against the version in `importmap.php`. If they do not match, the package is upgraded to a version that matches. <img width="995" alt="Screenshot 2023-10-22 at 1 45 41 PM" src="https://github.com/symfony/flex/assets/121003/286e58b0-d73d-4d7a-bc19-f748b18f489b"> We're not creating a fully-fledge package management by any means, but we don't need to. With a few notifications and things like this, we can keep the user's dependencies in sync with each other. Unrelated: don't forget about that really cool other PR symfony#978 ;) Cheers! Commits ------- 5214615 AssetMapper: upgrade packages when needed
Fan2Shrek · Oct 22, 2023 · 884a881 · 884a881
2 parents 333abcf + 5214615
commit 884a881
Show file tree

Hide file tree

Showing 8 changed files with 98 additions and 38 deletions.
diff --git a/src/Flex.php b/src/Flex.php
@@ -600,7 +600,7 @@ private function synchronizePackageJson(string $rootDir)
         $vendorDir = trim((new Filesystem())->makePathRelative($this->config->get('vendor-dir'), $rootDir), '/');
 
         $executor = new ScriptExecutor($this->composer, $this->io, $this->options);
-        $synchronizer = new PackageJsonSynchronizer($rootDir, $vendorDir, $executor);
+        $synchronizer = new PackageJsonSynchronizer($rootDir, $vendorDir, $executor, $this->io);
 
         if ($synchronizer->shouldSynchronize()) {
             $lockData = $this->composer->getLocker()->getLockData();

diff --git a/src/PackageJsonSynchronizer.php b/src/PackageJsonSynchronizer.php
@@ -11,8 +11,10 @@
 
 namespace Symfony\Flex;
 
+use Composer\IO\IOInterface;
 use Composer\Json\JsonFile;
 use Composer\Json\JsonManipulator;
+use Composer\Semver\Semver;
 use Composer\Semver\VersionParser;
 use Seld\JsonLint\ParsingException;
 
@@ -25,13 +27,15 @@ class PackageJsonSynchronizer
     private $rootDir;
     private $vendorDir;
     private $scriptExecutor;
+    private $io;
     private $versionParser;
 
-    public function __construct(string $rootDir, string $vendorDir, ScriptExecutor $scriptExecutor)
+    public function __construct(string $rootDir, string $vendorDir, ScriptExecutor $scriptExecutor, IOInterface $io)
     {
         $this->rootDir = $rootDir;
         $this->vendorDir = $vendorDir;
         $this->scriptExecutor = $scriptExecutor;
+        $this->io = $io;
         $this->versionParser = new VersionParser();
     }
 
@@ -147,11 +151,9 @@ private function resolveImportMapPackages($phpPackage): array
         foreach ($packageJson->read()['symfony']['importmap'] ?? [] as $importMapName => $constraintConfig) {
             if (\is_array($constraintConfig)) {
                 $constraint = $constraintConfig['version'] ?? [];
-                $preload = $constraintConfig['preload'] ?? false;
                 $package = $constraintConfig['package'] ?? $importMapName;
             } else {
                 $constraint = $constraintConfig;
-                $preload = false;
                 $package = $importMapName;
             }
 
@@ -161,7 +163,6 @@ private function resolveImportMapPackages($phpPackage): array
 
                 $dependencies[$importMapName] = [
                     'path' => $path,
-                    'preload' => $preload,
                 ];
 
                 continue;
@@ -170,7 +171,6 @@ private function resolveImportMapPackages($phpPackage): array
             $dependencies[$importMapName] = [
                 'version' => $constraint,
                 'package' => $package,
-                'preload' => $preload,
             ];
         }
 
@@ -239,7 +239,7 @@ private function shouldUpdateConstraint(string $existingConstraint, string $cons
     }
 
     /**
-     * @param array<string, array{path?: string, preload: bool, package?: string, version?: string}> $importMapEntries
+     * @param array<string, array{path?: string, package?: string, version?: string}> $importMapEntries
      */
     private function updateImportMap(array $importMapEntries): void
     {
@@ -251,14 +251,24 @@ private function updateImportMap(array $importMapEntries): void
 
         foreach ($importMapEntries as $name => $importMapEntry) {
             if (isset($importMapData[$name])) {
-                continue;
+                if (!isset($importMapData[$name]['version'])) {
+                    // AssetMapper 6.3
+                    continue;
+                }
+
+                $version = $importMapData[$name]['version'];
+                $versionConstraint = $importMapEntry['version'] ?? null;
+
+                // if the version constraint is satisfied, skip - else, update the package
+                if (Semver::satisfies($version, $versionConstraint)) {
+                    continue;
+                }
+
+                $this->io->writeError(sprintf('Updating package <comment>%s</> from <info>%s</> to <info>%s</>.', $name, $version, $versionConstraint));
             }
 
             if (isset($importMapEntry['path'])) {
                 $arguments = [$name, '--path='.$importMapEntry['path']];
-                if ($importMapEntry['preload']) {
-                    $arguments[] = '--preload';
-                }
                 $this->scriptExecutor->execute(
                     'symfony-cmd',
                     'importmap:require',
@@ -274,9 +284,6 @@ private function updateImportMap(array $importMapEntries): void
                     $packageName .= '='.$name;
                 }
                 $arguments = [$packageName];
-                if ($importMapEntry['preload']) {
-                    $arguments[] = '--preload';
-                }
                 $this->scriptExecutor->execute(
                     'symfony-cmd',
                     'importmap:require',

diff --git a/tests/Fixtures/packageJson/elevated_dependencies_package.json b/tests/Fixtures/packageJson/elevated_dependencies_package.json
@@ -1,8 +1,8 @@
 {
    "name": "symfony/fixture",
    "dependencies": {
-      "@hotcookies": "^1.1|^2",
-      "@hotdogs": "^2",
+      "@hotcookies/bar": "^1.1|^2",
+      "@hotdogs/bun": "^2",
       "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets"
    },
    "devDependencies": {

diff --git a/tests/Fixtures/packageJson/stricter_constraints_package.json b/tests/Fixtures/packageJson/stricter_constraints_package.json
@@ -1,8 +1,8 @@
 {
    "name": "symfony/fixture",
    "devDependencies": {
-      "@hotcookies": "^2",
-      "@hotdogs": "^1.9",
+      "@hotcookies/bar": "^2",
+      "@hotdogs/bun": "^1.9",
       "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets"
    },
    "browserslist": [

diff --git a/tests/Fixtures/packageJson/vendor/symfony/existing-package/Resources/assets/package.json b/tests/Fixtures/packageJson/vendor/symfony/existing-package/Resources/assets/package.json
@@ -12,7 +12,7 @@
         }
     },
     "peerDependencies": {
-        "@hotcookies": "^1.1|^2",
-        "@hotdogs": "^2"
+        "@hotcookies/bar": "^1.1|^2",
+        "@hotdogs/bun": "^2"
     }
 }
diff --git a/tests/Fixtures/packageJson/vendor/symfony/new-package/assets/package.json b/tests/Fixtures/packageJson/vendor/symfony/new-package/assets/package.json
@@ -11,14 +11,13 @@
         },
         "entrypoints": ["admin.js"],
         "importmap": {
-            "@hotcake": "^1.9.0",
+            "@hotcake/foo": "^1.9.0",
             "@symfony/new-package": {
-                "version": "path:%PACKAGE%/dist/loader.js",
-                "preload": true
+                "version": "path:%PACKAGE%/dist/loader.js"
             }
         }
     },
     "peerDependencies": {
-        "@hotcookies": "^1.1"
+        "@hotcookies/bar": "^1.1"
     }
 }
diff --git a/tests/Fixtures/packageJson/vendor/symfony/package-no-file-package/assets/package.json b/tests/Fixtures/packageJson/vendor/symfony/package-no-file-package/assets/package.json
@@ -3,6 +3,6 @@
         "needsPackageAsADependency": false
     },
     "peerDependencies": {
-        "@hotcookies": "^1.1"
+        "@hotcookies/bar": "^1.1"
     }
 }
diff --git a/tests/PackageJsonSynchronizerTest.php b/tests/PackageJsonSynchronizerTest.php
@@ -11,6 +11,7 @@
 
 namespace Symfony\Flex\Tests;
 
+use Composer\IO\IOInterface;
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\Filesystem\Filesystem;
 use Symfony\Flex\PackageJsonSynchronizer;
@@ -31,7 +32,8 @@ protected function setUp(): void
         $this->synchronizer = new PackageJsonSynchronizer(
             $this->tempDir,
             'vendor',
-            $this->scriptExecutor
+            $this->scriptExecutor,
+            $this->createMock(IOInterface::class)
         );
     }
 
@@ -99,8 +101,8 @@ public function testSynchronizeExistingPackage()
             [
                 'name' => 'symfony/fixture',
                 'devDependencies' => [
-                    '@hotcookies' => '^1.1|^2',
-                    '@hotdogs' => '^2',
+                    '@hotcookies/bar' => '^1.1|^2',
+                    '@hotdogs/bun' => '^2',
                     '@symfony/existing-package' => 'file:vendor/symfony/existing-package/Resources/assets',
                     '@symfony/stimulus-bridge' => '^1.0.0',
                     'stimulus' => '^1.1.1',
@@ -151,7 +153,7 @@ public function testSynchronizeNewPackage()
             '{
    "name": "symfony/fixture",
    "devDependencies": {
-      "@hotdogs": "^2",
+      "@hotdogs/bun": "^2",
       "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets",
       "@symfony/new-package": "file:vendor/symfony/new-package/assets",
       "@symfony/stimulus-bridge": "^1.0.0",
@@ -207,8 +209,8 @@ public function testArrayFormattingHasNotChanged()
             '{
    "name": "symfony/fixture",
    "devDependencies": {
-      "@hotcookies": "^1.1|^2",
-      "@hotdogs": "^2",
+      "@hotcookies/bar": "^1.1|^2",
+      "@hotdogs/bun": "^2",
       "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets",
       "@symfony/stimulus-bridge": "^1.0.0",
       "stimulus": "^1.1.1"
@@ -237,8 +239,8 @@ public function testExistingElevatedPackage()
             [
                 'name' => 'symfony/fixture',
                 'dependencies' => [
-                    '@hotcookies' => '^1.1|^2',
-                    '@hotdogs' => '^2',
+                    '@hotcookies/bar' => '^1.1|^2',
+                    '@hotdogs/bun' => '^2',
                     '@symfony/existing-package' => 'file:vendor/symfony/existing-package/Resources/assets',
                 ],
                 'devDependencies' => [
@@ -272,9 +274,9 @@ public function testStricterConstraintsAreKeptNonMatchingAreReplaced()
                 'name' => 'symfony/fixture',
                 'devDependencies' => [
                     // this satisfies the constraint, so it's kept
-                    '@hotcookies' => '^2',
+                    '@hotcookies/bar' => '^2',
                     // this was too low, so it's replaced
-                    '@hotdogs' => '^2',
+                    '@hotdogs/bun' => '^2',
                     '@symfony/existing-package' => 'file:vendor/symfony/existing-package/Resources/assets',
                 ],
                 'browserslist' => [
@@ -303,7 +305,7 @@ public function testSynchronizePackageWithoutNeedingFilePackage()
             '{
    "name": "symfony/fixture",
    "devDependencies": {
-      "@hotdogs": "^2",
+      "@hotdogs/bun": "^2",
       "@symfony/existing-package": "file:vendor/symfony/existing-package/Resources/assets",
       "@symfony/stimulus-bridge": "^1.0.0",
       "stimulus": "^1.1.1"
@@ -324,12 +326,13 @@ public function testSynchronizeAssetMapperNewPackage()
         $this->scriptExecutor->expects($this->exactly(2))
             ->method('execute')
             ->withConsecutive(
-                ['symfony-cmd', 'importmap:require', ['@hotcake@^1.9.0']],
-                ['symfony-cmd', 'importmap:require', ['@symfony/new-package', '--path='.$fileModulePath, '--preload']]
+                ['symfony-cmd', 'importmap:require', ['@hotcake/foo@^1.9.0']],
+                ['symfony-cmd', 'importmap:require', ['@symfony/new-package', '--path='.$fileModulePath]]
             );
 
         $this->synchronizer->synchronize([
             [
+                // no "importmap" specific config, but still registered as a controller
                 'name' => 'symfony/existing-package',
                 'keywords' => ['symfony-ux'],
             ],
@@ -384,4 +387,55 @@ public function testSynchronizeAssetMapperNewPackage()
             json_decode(file_get_contents($this->tempDir.'/assets/controllers.json'), true)
         );
     }
+
+    public function testSynchronizeAssetMapperUpgradesPackageIfNeeded()
+    {
+        $importMap = [
+            '@hotcake/foo' => [
+                // constraint in package.json is ^1.9.0
+                'version' => '1.8.0',
+            ],
+        ];
+        file_put_contents($this->tempDir.'/importmap.php', sprintf('<?php return %s;', var_export($importMap, true)));
+
+        $fileModulePath = $this->tempDir.'/vendor/symfony/new-package/assets/dist/loader.js';
+        $this->scriptExecutor->expects($this->exactly(2))
+            ->method('execute')
+            ->withConsecutive(
+                ['symfony-cmd', 'importmap:require', ['@hotcake/foo@^1.9.0']],
+                ['symfony-cmd', 'importmap:require', ['@symfony/new-package', '--path='.$fileModulePath]]
+            );
+
+        $this->synchronizer->synchronize([
+            [
+                'name' => 'symfony/new-package',
+                'keywords' => ['symfony-ux'],
+            ],
+        ]);
+    }
+
+    public function testSynchronizeAssetMapperSkipsUpgradeIfAlreadySatisfied()
+    {
+        $importMap = [
+            '@hotcake/foo' => [
+                // constraint in package.json is ^1.9.0
+                'version' => '1.9.1',
+            ],
+        ];
+        file_put_contents($this->tempDir.'/importmap.php', sprintf('<?php return %s;', var_export($importMap, true)));
+
+        $fileModulePath = $this->tempDir.'/vendor/symfony/new-package/assets/dist/loader.js';
+        $this->scriptExecutor->expects($this->once())
+            ->method('execute')
+            ->withConsecutive(
+                ['symfony-cmd', 'importmap:require', ['@symfony/new-package', '--path='.$fileModulePath]]
+            );
+
+        $this->synchronizer->synchronize([
+            [
+                'name' => 'symfony/new-package',
+                'keywords' => ['symfony-ux'],
+            ],
+        ]);
+    }
 }