Fixing bug where autoimport CSS from importmap did not use their impo…

…rtmap name
weaverryan · Nov 1, 2023 · 84b9a15 · 84b9a15
1 parent 1e68d62
commit 84b9a15
Show file tree

Hide file tree

Showing 19 changed files with 224 additions and 124 deletions.
diff --git a/src/StimulusBundle/src/AssetMapper/AutoImportLocator.php b/src/StimulusBundle/src/AssetMapper/AutoImportLocator.php
@@ -28,11 +28,11 @@ public function __construct(
     }
 
     // parts of this method are duplicated & adapted from UxControllersTwigRuntime
-    public function locateAsset(string $path, UxPackageMetadata $packageMetadata): ?MappedAsset
+    public function locateAutoImport(string $path, UxPackageMetadata $packageMetadata): MappedControllerAutoImport
     {
         // see if this is a mapped asset path
         if ($asset = $this->assetMapper->getAsset($path)) {
-            return $asset;
+            return new MappedControllerAutoImport($asset->sourcePath, false);
         }
 
         $slashPosition = strpos($path, '/');
@@ -59,22 +59,14 @@ public function locateAsset(string $path, UxPackageMetadata $packageMetadata): ?
                 throw new \LogicException(sprintf('An "autoimport" in "controllers.json" refers to "%s". This file was found, but the path is not in the asset mapper. And so, the file cannot be loaded. This is a misconfiguration with the bundle providing this.', $path));
             }
 
-            return $asset;
+            return new MappedControllerAutoImport($asset->sourcePath, false);
         }
 
         $entry = $this->importMapConfigReader->findRootImportMapEntry($path);
         if (!$entry) {
             throw new \LogicException(sprintf('The autoimport "%s" could not be found in importmap.php. Try running "importmap:require %s".', $path, $path));
         }
 
-        if ($asset = $this->assetMapper->getAsset($entry->path)) {
-            return $asset;
-        }
-
-        if ($asset = $this->assetMapper->getAssetFromSourcePath($entry->path)) {
-            return $asset;
-        }
-
-        throw new \LogicException(sprintf('The autoimport "%s" is correctly in importmap.php but the file could not be found. Try running the "importmap:install" command.', $path));
+        return new MappedControllerAutoImport($path, true);
     }
 }
diff --git a/src/StimulusBundle/src/AssetMapper/ControllersMapGenerator.php b/src/StimulusBundle/src/AssetMapper/ControllersMapGenerator.php
@@ -142,6 +142,9 @@ private function loadUxControllers(): array
         return $controllersMap;
     }
 
+    /**
+     * @return MappedControllerAutoImport[]
+     */
     private function collectAutoImports(array $autoImports, UxPackageMetadata $currentPackageMetadata): array
     {
         // @legacy: Backwards compatibility with Symfony 6.3
@@ -152,15 +155,15 @@ private function collectAutoImports(array $autoImports, UxPackageMetadata $curre
             throw new \InvalidArgumentException(sprintf('The "autoImportLocator" argument to "%s" is required when using AssetMapper 6.4', self::class));
         }
 
-        $autoImportAssets = [];
+        $autoImportItems = [];
         foreach ($autoImports as $path => $enabled) {
             if (!$enabled) {
                 continue;
             }
 
-            $autoImportAssets[] = $this->autoImportLocator->locateAsset($path, $currentPackageMetadata);
+            $autoImportItems[] = $this->autoImportLocator->locateAutoImport($path, $currentPackageMetadata);
         }
 
-        return $autoImportAssets;
+        return $autoImportItems;
     }
 }
diff --git a/src/StimulusBundle/src/AssetMapper/MappedControllerAsset.php b/src/StimulusBundle/src/AssetMapper/MappedControllerAsset.php
@@ -24,7 +24,7 @@ public function __construct(
         public MappedAsset $asset,
         public bool $isLazy,
         /**
-         * @var MappedAsset[]
+         * @var MappedControllerAutoImport[]
          */
         public array $autoImports = [],
     ) {

diff --git a/src/StimulusBundle/src/AssetMapper/MappedControllerAutoImport.php b/src/StimulusBundle/src/AssetMapper/MappedControllerAutoImport.php
@@ -0,0 +1,26 @@
+<?php
+
+/*
+ * This file is part of the Symfony package.
+ *
+ * (c) Fabien Potencier <[email protected]>
+ *
+ * For the full copyright and license information, please view the LICENSE
+ * file that was distributed with this source code.
+ */
+
+namespace Symfony\UX\StimulusBundle\AssetMapper;
+
+/**
+ * @experimental
+ *
+ * @author Ryan Weaver <[email protected]>
+ */
+class MappedControllerAutoImport
+{
+    public function __construct(
+        public string $path,
+        public bool $isBareImport
+    ) {
+    }
+}
diff --git a/src/StimulusBundle/src/AssetMapper/StimulusLoaderJavaScriptCompiler.php b/src/StimulusBundle/src/AssetMapper/StimulusLoaderJavaScriptCompiler.php
@@ -80,9 +80,13 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
                 $asset->addDependency($mappedControllerAsset->asset);
             }
 
-            $autoImportRelativePaths = [];
-            foreach ($mappedControllerAsset->autoImports as $autoImportAsset) {
-                $autoImportRelativePaths[] = json_encode(Path::makeRelative($autoImportAsset->sourcePath, \dirname($asset->sourcePath)), \JSON_THROW_ON_ERROR | \JSON_UNESCAPED_SLASHES);
+            $autoImportPaths = [];
+            foreach ($mappedControllerAsset->autoImports as $autoImport) {
+                if ($autoImport->isBareImport) {
+                    $autoImportPaths[] = json_encode($autoImport->path, \JSON_THROW_ON_ERROR | \JSON_UNESCAPED_SLASHES);
+                } else {
+                    $autoImportPaths[] = json_encode(Path::makeRelative($autoImport->path, \dirname($asset->sourcePath)), \JSON_THROW_ON_ERROR | \JSON_UNESCAPED_SLASHES);
+                }
             }
 
             if ($mappedControllerAsset->isLazy) {
@@ -91,7 +95,7 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
                 } else {
                     // import $relativeImportPath and also the auto-imports
                     // and use a Promise.all() to wait for all of them
-                    $lazyControllers[] = sprintf('%s: () => Promise.all([import(%s), %s]).then((ret) => ret[0])', json_encode($name), $relativeImportPath, implode(', ', array_map(fn ($path) => "import($path)", $autoImportRelativePaths)));
+                    $lazyControllers[] = sprintf('%s: () => Promise.all([import(%s), %s]).then((ret) => ret[0])', json_encode($name), $relativeImportPath, implode(', ', array_map(fn ($path) => "import($path)", $autoImportPaths)));
                 }
 
                 continue;
@@ -104,7 +108,7 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
                 $controllerNameForVariable,
                 $relativeImportPath
             );
-            foreach ($autoImportRelativePaths as $autoImportRelativePath) {
+            foreach ($autoImportPaths as $autoImportRelativePath) {
                 $importLines[] = sprintf(
                     'import %s;',
                     $autoImportRelativePath

diff --git a/src/StimulusBundle/tests/AssetMapper/AutoImportLocatorTest.php b/src/StimulusBundle/tests/AssetMapper/AutoImportLocatorTest.php
@@ -29,24 +29,25 @@ protected function setUp(): void
         }
     }
 
-    public function testLocateAssetCanHandleAssetMapperPath()
+    public function testLocateAutoImportCanHandleAssetMapperPath()
     {
         $assetMapper = $this->createMock(AssetMapperInterface::class);
-        $asset = new MappedAsset('foo.css');
         $assetMapper->expects($this->once())
             ->method('getAsset')
             ->with('foo.css')
-            ->willReturn($asset);
+            ->willReturn(new MappedAsset('foo.css', '/path/to/foo.css'));
 
         $locator = new AutoImportLocator(
             $this->createMock(ImportMapConfigReader::class),
             $assetMapper,
         );
         $packageMetadata = new UxPackageMetadata('foo', [], 'bar');
-        $this->assertSame($asset, $locator->locateAsset('foo.css', $packageMetadata));
+        $autoImport = $locator->locateAutoImport('foo.css', $packageMetadata);
+        $this->assertSame('/path/to/foo.css', $autoImport->path);
+        $this->assertFalse($autoImport->isBareImport);
     }
 
-    public function testLocateAssetHandlesFileInPackage()
+    public function testLocateAutoImportHandlesFileInPackage()
     {
         $packageMetadata = new UxPackageMetadata(
             __DIR__.'/../fixtures/vendor/fake-vendor/ux-package1/assets',
@@ -55,78 +56,37 @@ public function testLocateAssetHandlesFileInPackage()
         );
 
         $assetMapper = $this->createMock(AssetMapperInterface::class);
-        $asset = new MappedAsset('styles.css');
         $assetMapper->expects($this->once())
             ->method('getAssetFromSourcePath')
             ->with(__DIR__.'/../fixtures/vendor/fake-vendor/ux-package1/assets/dist/styles.css')
-            ->willReturn($asset);
+            ->willReturn(new MappedAsset('styles.css', '/path/to/styles.css'));
 
         $locator = new AutoImportLocator(
             $this->createMock(ImportMapConfigReader::class),
             $assetMapper,
         );
 
-        $actualAsset = $locator->locateAsset('@fake-vendor/ux-package1/dist/styles.css', $packageMetadata);
-        $this->assertSame($asset, $actualAsset);
+        $autoImport = $locator->locateAutoImport('@fake-vendor/ux-package1/dist/styles.css', $packageMetadata);
+        $this->assertSame('/path/to/styles.css', $autoImport->path);
+        $this->assertFalse($autoImport->isBareImport);
     }
 
-    public function testLocateAssetViaLogicalPathFromImportMap()
+    public function testLocateAutoImportFromImportMap()
     {
         $importMapConfigReader = $this->createMock(ImportMapConfigReader::class);
         $importMapConfigReader->expects($this->once())
             ->method('findRootImportMapEntry')
             ->with('tom-select/dist/css/tom-select.default.css')
             ->willReturn(ImportMapEntry::createRemote('tom-select/dist/css/tom-select.default.css', ImportMapType::CSS, '/path/to/vendor/tom-select.default.css', '1.0.0', 'tom-select/dist/css/tom-select.default.css', false));
 
-        $assetMapper = $this->createMock(AssetMapperInterface::class);
-        $asset = new MappedAsset('styles.css');
-        $assetMapper->expects($this->any())
-            ->method('getAsset')
-            ->willReturnCallback(function (string $path) use ($asset) {
-                // don't find the actual auto import path
-                if ('tom-select/dist/css/tom-select.default.css' === $path) {
-                    return null;
-                }
-
-                if ('/path/to/vendor/tom-select.default.css' === $path) {
-                    return $asset;
-                }
-
-                throw new \LogicException('Unexpected path: '.$path);
-            });
-
-        $locator = new AutoImportLocator(
-            $importMapConfigReader,
-            $assetMapper,
-        );
-
-        $packageMetadata = new UxPackageMetadata('foo', [], 'bar');
-        $actualAsset = $locator->locateAsset('tom-select/dist/css/tom-select.default.css', $packageMetadata);
-        $this->assertSame($asset, $actualAsset);
-    }
-
-    public function testLocateAssetViaSourcePathFromImportMap()
-    {
-        $importMapConfigReader = $this->createMock(ImportMapConfigReader::class);
-        $importMapConfigReader->expects($this->once())
-            ->method('findRootImportMapEntry')
-            ->with('tom-select/dist/css/tom-select.default.css')
-            ->willReturn(ImportMapEntry::createRemote('tom-select/dist/css/tom-select.default.css', ImportMapType::CSS, '/path/to/vendor/tom-select.default.css', '1.0.0', 'tom-select/dist/css/tom-select.default.css', false));
-
-        $assetMapper = $this->createMock(AssetMapperInterface::class);
-        $asset = new MappedAsset('styles.css');
-        $assetMapper->expects($this->any())
-            ->method('getAssetFromSourcePath')
-            ->with('/path/to/vendor/tom-select.default.css')
-            ->willReturn($asset);
-
         $locator = new AutoImportLocator(
             $importMapConfigReader,
-            $assetMapper,
+            $this->createMock(AssetMapperInterface::class),
         );
 
         $packageMetadata = new UxPackageMetadata('foo', [], 'bar');
-        $actualAsset = $locator->locateAsset('tom-select/dist/css/tom-select.default.css', $packageMetadata);
-        $this->assertSame($asset, $actualAsset);
+        $autoImport = $locator->locateAutoImport('tom-select/dist/css/tom-select.default.css', $packageMetadata);
+        $this->assertSame('tom-select/dist/css/tom-select.default.css', $autoImport->path);
+        $this->assertTrue($autoImport->isBareImport);
     }
 }
diff --git a/src/StimulusBundle/tests/AssetMapper/ControllerMapGeneratorTest.php b/src/StimulusBundle/tests/AssetMapper/ControllerMapGeneratorTest.php
@@ -17,6 +17,7 @@
 use Symfony\Component\AssetMapper\MappedAsset;
 use Symfony\UX\StimulusBundle\AssetMapper\AutoImportLocator;
 use Symfony\UX\StimulusBundle\AssetMapper\ControllersMapGenerator;
+use Symfony\UX\StimulusBundle\AssetMapper\MappedControllerAutoImport;
 use Symfony\UX\StimulusBundle\Ux\UxPackageReader;
 
 class ControllerMapGeneratorTest extends TestCase
@@ -48,14 +49,14 @@ public function testGetControllersMap()
         $autoImportLocator = $this->createMock(AutoImportLocator::class);
         if (class_exists(ImportMapConfigReader::class)) {
             $autoImportLocator->expects($this->any())
-                ->method('locateAsset')
+                ->method('locateAutoImport')
                 ->willReturnCallback(function ($path) {
-                    return new MappedAsset($path, '/path/to'.$path);
+                    return new MappedControllerAutoImport('/path/to'.$path, false);
                 });
         } else {
             // @legacy for AssetMapper 6.3
             $autoImportLocator->expects($this->never())
-                ->method('locateAsset');
+                ->method('locateAutoImport');
         }
 
         $generator = new ControllersMapGenerator(