bug symfony#52523 [AssetMapper] avoid caching MappedAsset inside JavaScript Import (weaverryan)

This PR was merged into the 6.4 branch.

Discussion
----------

[AssetMapper] avoid caching MappedAsset inside JavaScript Import

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | None
| License       | MIT

Hi!

Subtle bug. In dev, we cache the `MappedAsset` objects. Before this PR, `MappedAsset->javascriptImports[0]->asset` was also a `MappedAsset` object.

Suppose `app.js` imports `foo.js` which imports `bar.js`. Then, the user changes `foo.js` to also import `baz.js`.

The `foo.js` MappedAsset will now have TWO `JavaScriptImport` objects (for `bar.js` and `baz.js`). However, when we calculate the preloads, this happens:

1) We load the cached `MappedAsset` for `app.js` because this file has not changed
2) It correctly has 1 `JavaScriptImport` for `foo.js`
3) But the `JavaScriptImport->asset` property holds an OUT-OF-DATE version of the `MappedAsset` for `foo.js`... because it was cached inside the `MappedAsset` structure for `app.js`. This out-of-date `MappedAsset` for `foo.js` only contains the ONE JavaScript import.

The result is that `baz.js` will be missing from the preloads.

This PR breaks the structure by only storing the asset's logical path (and sourcePath - needed by the cache system). Then, at runtime, we load the fresh `MappedAsset` from the logical path.

Cheers!

Commits
-------

bb5ef88 [AssetMapper] Avoid storing & caching MappedAsset objects

Author: nicolas-grekas

src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.php

@@ -72,7 +72,8 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
             $addToImportMap = $isRelativeImport;
             $asset->addJavaScriptImport(new JavaScriptImport(
                 $addToImportMap ? $dependentAsset->publicPathWithoutDigest : $importedModule,
-                $dependentAsset,
+                $dependentAsset->logicalPath,
+                $dependentAsset->sourcePath,
                 $isLazy,
                 $addToImportMap,
             ));

src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php

@@ -69,7 +69,7 @@ private function collectResourcesFromAsset(MappedAsset $mappedAsset): array
         }
 
         foreach ($mappedAsset->getJavaScriptImports() as $import) {
-            $resources[] = new FileExistenceResource($import->asset->sourcePath);
+            $resources[] = new FileExistenceResource($import->assetSourcePath);
         }
 
         return $resources;

src/Symfony/Component/AssetMapper/ImportMap/ImportMapGenerator.php

@@ -179,10 +179,15 @@ private function addImplicitEntries(ImportMapEntry $entry, array $currentImportE
 
             // check if this import requires an automatic importmap entry
             if ($javaScriptImport->addImplicitlyToImportMap) {
+                if (!$importedAsset = $this->assetMapper->getAsset($javaScriptImport->assetLogicalPath)) {
+                    // should not happen at this point, unless something added a bogus JavaScriptImport to this asset
+                    throw new LogicException(sprintf('Cannot find imported JavaScript asset "%s" in asset mapper.', $javaScriptImport->assetLogicalPath));
+                }
+
                 $nextEntry = ImportMapEntry::createLocal(
                     $importName,
-                    ImportMapType::tryFrom($javaScriptImport->asset->publicExtension) ?: ImportMapType::JS,
-                    $javaScriptImport->asset->logicalPath,
+                    ImportMapType::tryFrom($importedAsset->publicExtension) ?: ImportMapType::JS,
+                    $importedAsset->logicalPath,
                     false,
                 );
 
@@ -223,7 +228,11 @@ private function findEagerImports(MappedAsset $asset): array
             $dependencies[] = $javaScriptImport->importName;
 
             // Follow its imports!
-            $dependencies = array_merge($dependencies, $this->findEagerImports($javaScriptImport->asset));
+            if (!$nextAsset = $this->assetMapper->getAsset($javaScriptImport->assetLogicalPath)) {
+                // should not happen at this point, unless something added a bogus JavaScriptImport to this asset
+                throw new LogicException(sprintf('Cannot find imported JavaScript asset "%s" in asset mapper.', $javaScriptImport->assetLogicalPath));
+            }
+            $dependencies = array_merge($dependencies, $this->findEagerImports($nextAsset));
         }
 
         return $dependencies;

src/Symfony/Component/AssetMapper/ImportMap/JavaScriptImport.php

@@ -11,21 +11,20 @@
 
 namespace Symfony\Component\AssetMapper\ImportMap;
 
-use Symfony\Component\AssetMapper\MappedAsset;
-
 /**
  * Represents a module that was imported by a JavaScript file.
  */
 final class JavaScriptImport
 {
     /**
-     * @param string      $importName               The name of the import needed in the importmap, e.g. "/foo.js" or "react"
-     * @param MappedAsset $asset                    The asset that was imported
-     * @param bool        $addImplicitlyToImportMap Whether this import should be added to the importmap automatically
+     * @param string $importName               The name of the import needed in the importmap, e.g. "/foo.js" or "react"
+     * @param string $assetLogicalPath         Logical path to the mapped ass that was imported
+     * @param bool   $addImplicitlyToImportMap Whether this import should be added to the importmap automatically
      */
     public function __construct(
         public readonly string $importName,
-        public readonly MappedAsset $asset,
+        public readonly string $assetLogicalPath,
+        public readonly string $assetSourcePath,
         public readonly bool $isLazy = false,
         public bool $addImplicitlyToImportMap = false,
     ) {

src/Symfony/Component/AssetMapper/MappedAsset.php

@@ -94,6 +94,8 @@ public function __construct(
     }
 
     /**
+     * Assets that the content of this asset depends on - for internal caching.
+     *
      * @return MappedAsset[]
      */
     public function getDependencies(): array

src/Symfony/Component/AssetMapper/Tests/Compiler/JavaScriptImportPathCompilerTest.php

@@ -58,7 +58,7 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
             ->method('getAsset')
             ->willReturnCallback(function ($path) {
                 return match ($path) {
-                    'module_in_importmap_local_asset.js' => new MappedAsset('module_in_importmap_local_asset.js', publicPathWithoutDigest: '/assets/module_in_importmap_local_asset.js'),
+                    'module_in_importmap_local_asset.js' => new MappedAsset('module_in_importmap_local_asset.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/module_in_importmap_local_asset.js'),
                     default => null,
                 };
             });
@@ -67,11 +67,11 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
             ->method('getAssetFromSourcePath')
             ->willReturnCallback(function ($path) {
                 return match ($path) {
-                    '/project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
-                    '/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
-                    '/project/assets/styles.css' => new MappedAsset('styles.css', publicPathWithoutDigest: '/assets/styles.css'),
-                    '/project/assets/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
-                    '/project/assets/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
+                    '/project/assets/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
+                    '/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
+                    '/project/assets/styles.css' => new MappedAsset('styles.css', '/can/be/anything.js', publicPathWithoutDigest: '/assets/styles.css'),
+                    '/project/assets/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
+                    '/project/assets/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
                     default => null,
                 };
             });
@@ -81,7 +81,7 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
         $this->assertSame($input, $compiler->compile($input, $asset, $assetMapper));
         $actualImports = [];
         foreach ($asset->getJavaScriptImports() as $import) {
-            $actualImports[$import->importName] = ['lazy' => $import->isLazy, 'asset' => $import->asset->logicalPath, 'add' => $import->addImplicitlyToImportMap];
+            $actualImports[$import->importName] = ['lazy' => $import->isLazy, 'asset' => $import->assetLogicalPath, 'add' => $import->addImplicitlyToImportMap];
         }
 
         $this->assertEquals($expectedJavaScriptImports, $actualImports);
@@ -304,9 +304,9 @@ public function testCompileFindsRelativePathsViaSourcePath()
             ->method('getAssetFromSourcePath')
             ->willReturnCallback(function ($path) {
                 return match ($path) {
-                    '/project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
-                    '/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
-                    '/project/root_asset.js' => new MappedAsset('root_asset.js', publicPathWithoutDigest: '/assets/root_asset.js'),
+                    '/project/assets/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
+                    '/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
+                    '/project/root_asset.js' => new MappedAsset('root_asset.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/root_asset.js'),
                     default => throw new \RuntimeException(sprintf('Unexpected source path "%s"', $path)),
                 };
             });
@@ -320,9 +320,9 @@ public function testCompileFindsRelativePathsViaSourcePath()
         $compiler = new JavaScriptImportPathCompiler($this->createMock(ImportMapConfigReader::class));
         $compiler->compile($input, $inputAsset, $assetMapper);
         $this->assertCount(3, $inputAsset->getJavaScriptImports());
-        $this->assertSame('other.js', $inputAsset->getJavaScriptImports()[0]->asset->logicalPath);
-        $this->assertSame('subdir/foo.js', $inputAsset->getJavaScriptImports()[1]->asset->logicalPath);
-        $this->assertSame('root_asset.js', $inputAsset->getJavaScriptImports()[2]->asset->logicalPath);
+        $this->assertSame('other.js', $inputAsset->getJavaScriptImports()[0]->assetLogicalPath);
+        $this->assertSame('subdir/foo.js', $inputAsset->getJavaScriptImports()[1]->assetLogicalPath);
+        $this->assertSame('root_asset.js', $inputAsset->getJavaScriptImports()[2]->assetLogicalPath);
     }
 
     public function testCompileFindsRelativePathsWithWindowsPathsViaSourcePath()
@@ -337,9 +337,9 @@ public function testCompileFindsRelativePathsWithWindowsPathsViaSourcePath()
             ->method('getAssetFromSourcePath')
             ->willReturnCallback(function ($path) {
                 return match ($path) {
-                    'C://project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
-                    'C://project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
-                    'C://project/root_asset.js' => new MappedAsset('root_asset.js', publicPathWithoutDigest: '/assets/root_asset.js'),
+                    'C://project/assets/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
+                    'C://project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
+                    'C://project/root_asset.js' => new MappedAsset('root_asset.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/root_asset.js'),
                     default => throw new \RuntimeException(sprintf('Unexpected source path "%s"', $path)),
                 };
             });
@@ -366,7 +366,7 @@ public function testImportPathsCanUpdateForDifferentPublicPath(string $input, st
         $asset = new MappedAsset('app.js', '/path/to/assets/app.js', publicPathWithoutDigest: $inputAssetPublicPath);
 
         $assetMapper = $this->createMock(AssetMapperInterface::class);
-        $importedAsset = new MappedAsset('anything', publicPathWithoutDigest: $importedPublicPath);
+        $importedAsset = new MappedAsset('anything', '/can/be/anything.js', publicPathWithoutDigest: $importedPublicPath);
         $assetMapper->expects($this->once())
             ->method('getAssetFromSourcePath')
             ->willReturn($importedAsset);
@@ -436,7 +436,7 @@ public function testCompileHandlesCircularRelativeAssets()
         $input = 'import "./other.js";';
         $compiler->compile($input, $appAsset, $assetMapper);
         $this->assertCount(1, $appAsset->getJavaScriptImports());
-        $this->assertSame($otherAsset, $appAsset->getJavaScriptImports()[0]->asset);
+        $this->assertSame($otherAsset->logicalPath, $appAsset->getJavaScriptImports()[0]->assetLogicalPath);
     }
 
     public function testCompileHandlesCircularBareImportAssets()
@@ -464,7 +464,7 @@ public function testCompileHandlesCircularBareImportAssets()
         $input = 'import "@popperjs/core";';
         $compiler->compile($input, $bootstrapAsset, $assetMapper);
         $this->assertCount(1, $bootstrapAsset->getJavaScriptImports());
-        $this->assertSame($popperAsset, $bootstrapAsset->getJavaScriptImports()[0]->asset);
+        $this->assertSame($popperAsset->logicalPath, $bootstrapAsset->getJavaScriptImports()[0]->assetLogicalPath);
     }
 
     /**
@@ -490,7 +490,7 @@ public function testMissingImportMode(string $sourceLogicalName, string $input,
             ->method('getAssetFromSourcePath')
             ->willReturnCallback(function ($sourcePath) {
                 return match ($sourcePath) {
-                    '/path/to/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
+                    '/path/to/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
                     default => null,
                 };
             }

src/Symfony/Component/AssetMapper/Tests/Factory/CachedMappedAssetFactoryTest.php

@@ -94,7 +94,7 @@ public function testAssetConfigCacheResourceContainsDependencies()
         $deeplyNestedAsset = new MappedAsset('file4.js', realpath(__DIR__.'/../Fixtures/dir2/file4.js'));
 
         $file6Asset = new MappedAsset('file6.js', realpath(__DIR__.'/../Fixtures/dir2/subdir/file6.js'));
-        $deeplyNestedAsset->addJavaScriptImport(new JavaScriptImport('file6', asset: $file6Asset));
+        $deeplyNestedAsset->addJavaScriptImport(new JavaScriptImport('file6', assetLogicalPath: $file6Asset->logicalPath, assetSourcePath: $file6Asset->sourcePath));
 
         $dependentOnContentAsset->addDependency($deeplyNestedAsset);
         $mappedAsset->addDependency($dependentOnContentAsset);