bug #52331 [AssetMapper] Fix file deleting errors & remove nullable M…

…appedAsset on JS import (weaverryan)

This PR was squashed before being merged into the 6.4 branch.

Discussion
----------

[AssetMapper] Fix file deleting errors & remove nullable MappedAsset on JS import

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

Hi!

This PR accomplishes 2 things:

A) If `app.js` imported `foo.js`, and you deleted `foo.js`, you got a big, unobvious error from Symfony. This was because the `MappedAsset` behind `app.js` remained cached, including the `JavaScriptImport` for `foo.js`. So then, the system was surprised when `app.js` had this `JavaScriptImport`... but the underlying file didn't exist. The fix was to add a `FileExistenceResource` for `foo.js`. We don't need to update the `app.js` cache if `foo.js` changes, but we DO need to update it if `foo.js` no longer exists (so that we can create a new `MappedAsset` without the import).

B) Upon looking at this, previously, `JavaScriptImport.asset` was nullable. That doesn't make sense, now that all vendor files are downloaded locally and exist in the AssetMapper. The `MappedAsset.javascriptImports` are used to implicitly add importmap entries for relative assets (which already required a `MappedAsset`) and to preload other dependencies. Previously, we added a `JavaScriptImport` with a null asset for things like absolute imports (`import 'https://example.com/foo.js') or bare imports that we couldn't find. But we can't preload things like this anyway - or we shouldn't bother to in the case of an absolute assets.

For (B), trying to tighten things up and do LESS, if we don't need it.

Cheers!

Commits
-------

f1708aa [AssetMapper] Fix file deleting errors & remove nullable MappedAsset on JS import

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

@@ -61,15 +61,19 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
                 $dependentAsset = $this->findAssetForRelativeImport($importedModule, $asset, $assetMapper);
             }
 
+            if (!$dependentAsset) {
+                return $fullImportString;
+            }
+
             // List as a JavaScript import.
             // This will cause the asset to be included in the importmap (for relative imports)
             // and will be used to generate the preloads in the importmap.
             $isLazy = str_contains($fullImportString, 'import(');
-            $addToImportMap = $isRelativeImport && $dependentAsset;
+            $addToImportMap = $isRelativeImport;
             $asset->addJavaScriptImport(new JavaScriptImport(
                 $addToImportMap ? $dependentAsset->publicPathWithoutDigest : $importedModule,
-                $isLazy,
                 $dependentAsset,
+                $isLazy,
                 $addToImportMap,
             ));
 

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

@@ -14,6 +14,7 @@
 use Symfony\Component\AssetMapper\MappedAsset;
 use Symfony\Component\Config\ConfigCache;
 use Symfony\Component\Config\Resource\DirectoryResource;
+use Symfony\Component\Config\Resource\FileExistenceResource;
 use Symfony\Component\Config\Resource\FileResource;
 use Symfony\Component\Config\Resource\ResourceInterface;
 
@@ -67,6 +68,10 @@ private function collectResourcesFromAsset(MappedAsset $mappedAsset): array
             $resources = array_merge($resources, $this->collectResourcesFromAsset($assetDependency));
         }
 
+        foreach ($mappedAsset->getJavaScriptImports() as $import) {
+            $resources[] = new FileExistenceResource($import->asset->sourcePath);
+        }
+
         return $resources;
     }
 }

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

@@ -178,7 +178,7 @@ private function addImplicitEntries(ImportMapEntry $entry, array $currentImportE
             }
 
             // check if this import requires an automatic importmap entry
-            if ($javaScriptImport->addImplicitlyToImportMap && $javaScriptImport->asset) {
+            if ($javaScriptImport->addImplicitlyToImportMap) {
                 $nextEntry = ImportMapEntry::createLocal(
                     $importName,
                     ImportMapType::tryFrom($javaScriptImport->asset->publicExtension) ?: ImportMapType::JS,
@@ -226,10 +226,8 @@ private function findEagerImports(MappedAsset $asset): array
 
             $dependencies[] = $javaScriptImport->importName;
 
-            // the import is for a MappedAsset? Follow its imports!
-            if ($javaScriptImport->asset) {
-                $dependencies = array_merge($dependencies, $this->findEagerImports($javaScriptImport->asset));
-            }
+            // Follow its imports!
+            $dependencies = array_merge($dependencies, $this->findEagerImports($javaScriptImport->asset));
         }
 
         return $dependencies;

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

@@ -19,19 +19,15 @@
 final class JavaScriptImport
 {
     /**
-     * @param string           $importName               The name of the import needed in the importmap, e.g. "/foo.js" or "react"
-     * @param bool             $isLazy                   Whether this import was lazy or eager
-     * @param MappedAsset|null $asset                    The asset that was imported, if known - needed to add to the importmap, also used to find further imports for preloading
-     * @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 MappedAsset $asset                    The asset 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 bool $isLazy = false,
-        public readonly ?MappedAsset $asset = null,
         public bool $addImplicitlyToImportMap = false,
     ) {
-        if (null === $asset && $addImplicitlyToImportMap) {
-            throw new \LogicException(sprintf('The "%s" import cannot be automatically added to the importmap without an asset.', $this->importName));
-        }
     }
 }

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

@@ -72,7 +72,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->asset->logicalPath, 'add' => $import->addImplicitlyToImportMap];
         }
 
         $this->assertEquals($expectedJavaScriptImports, $actualImports);
@@ -172,9 +172,10 @@ public static function provideCompileTests(): iterable
             'expectedJavaScriptImports' => ['/assets/styles.css' => ['lazy' => false, 'asset' => 'styles.css', 'add' => true]],
         ];
 
-        yield 'importing_non_existent_file_without_strict_mode_is_ignored_still_listed_as_an_import' => [
+        yield 'importing_non_existent_file_without_strict_mode_is_ignored_and_no_import_added' => [
+            'sourceLogicalName' => 'app.js',
             'input' => "import './non-existent.js';",
-            'expectedJavaScriptImports' => ['./non-existent.js' => ['lazy' => false, 'asset' => null, 'add' => false]],
+            'expectedJavaScriptImports' => [],
         ];
 
         yield 'single_line_comment_at_start_ignored' => [
@@ -262,7 +263,7 @@ public static function provideCompileTests(): iterable
 
         yield 'bare_import_not_in_importmap' => [
             'input' => 'import "some_module";',
-            'expectedJavaScriptImports' => ['some_module' => ['lazy' => false, 'asset' => null, 'add' => false]],
+            'expectedJavaScriptImports' => [],
         ];
 
         yield 'bare_import_in_importmap_with_local_asset' => [
@@ -275,9 +276,14 @@ public static function provideCompileTests(): iterable
             'expectedJavaScriptImports' => ['module_in_importmap_remote' => ['lazy' => false, 'asset' => 'module_in_importmap_remote.js', 'add' => false]],
         ];
 
+<<<<<<< HEAD
         yield 'absolute_import_added_as_dependency_only' => [
+=======
+        yield 'absolute_import_ignored_and_no_dependency_added' => [
+            'sourceLogicalName' => 'app.js',
+>>>>>>> a79f543f8f ([AssetMapper] Fix file deleting errors & remove nullable MappedAsset on JS import)
             'input' => 'import "https://example.com/module.js";',
-            'expectedJavaScriptImports' => ['https://example.com/module.js' => ['lazy' => false, 'asset' => null, 'add' => false]],
+            'expectedJavaScriptImports' => [],
         ];
 
         yield 'bare_import_with_minimal_spaces' => [

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

@@ -14,9 +14,11 @@
 use PHPUnit\Framework\TestCase;
 use Symfony\Component\AssetMapper\Factory\CachedMappedAssetFactory;
 use Symfony\Component\AssetMapper\Factory\MappedAssetFactoryInterface;
+use Symfony\Component\AssetMapper\ImportMap\JavaScriptImport;
 use Symfony\Component\AssetMapper\MappedAsset;
 use Symfony\Component\Config\ConfigCache;
 use Symfony\Component\Config\Resource\DirectoryResource;
+use Symfony\Component\Config\Resource\FileExistenceResource;
 use Symfony\Component\Config\Resource\FileResource;
 use Symfony\Component\Filesystem\Filesystem;
 
@@ -89,9 +91,11 @@ public function testAssetConfigCacheResourceContainsDependencies()
         $mappedAsset = new MappedAsset('file1.css', $sourcePath, content: 'cached content');
 
         $dependentOnContentAsset = new MappedAsset('file3.css', realpath(__DIR__.'/../Fixtures/dir2/file3.css'));
-
         $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));
+
         $dependentOnContentAsset->addDependency($deeplyNestedAsset);
         $mappedAsset->addDependency($dependentOnContentAsset);
 
@@ -112,14 +116,15 @@ public function testAssetConfigCacheResourceContainsDependencies()
         $cachedFactory->createMappedAsset('file1.css', $sourcePath);
 
         $configCacheMetadata = $this->loadConfigCacheMetadataFor($mappedAsset);
-        $this->assertCount(5, $configCacheMetadata);
+        $this->assertCount(6, $configCacheMetadata);
         $this->assertInstanceOf(FileResource::class, $configCacheMetadata[0]);
         $this->assertInstanceOf(DirectoryResource::class, $configCacheMetadata[1]);
         $this->assertInstanceOf(FileResource::class, $configCacheMetadata[2]);
         $this->assertSame(realpath(__DIR__.'/../Fixtures/importmap.php'), $configCacheMetadata[0]->getResource());
         $this->assertSame($mappedAsset->sourcePath, $configCacheMetadata[2]->getResource());
         $this->assertSame($dependentOnContentAsset->sourcePath, $configCacheMetadata[3]->getResource());
         $this->assertSame($deeplyNestedAsset->sourcePath, $configCacheMetadata[4]->getResource());
+        $this->assertInstanceOf(FileExistenceResource::class, $configCacheMetadata[5]);
     }
 
     private function loadConfigCacheMetadataFor(MappedAsset $mappedAsset): array

src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapGeneratorTest.php

@@ -139,25 +139,25 @@ public function testGetImportMapData()
                 'entry1.js',
                 publicPath: '/assets/entry1-d1g35t.js',
                 javaScriptImports: [
-                    new JavaScriptImport('/assets/imported_file1.js', isLazy: false, asset: $importedFile1, addImplicitlyToImportMap: true),
-                    new JavaScriptImport('/assets/styles/file1.css', isLazy: false, asset: $importedCss1, addImplicitlyToImportMap: true),
-                    new JavaScriptImport('normal_js_file', isLazy: false, asset: $normalJsFile),
+                    new JavaScriptImport('/assets/imported_file1.js', asset: $importedFile1, isLazy: false, addImplicitlyToImportMap: true),
+                    new JavaScriptImport('/assets/styles/file1.css', asset: $importedCss1, isLazy: false, addImplicitlyToImportMap: true),
+                    new JavaScriptImport('normal_js_file', asset: $normalJsFile, isLazy: false),
                 ]
             ),
             new MappedAsset(
                 'entry2.js',
                 publicPath: '/assets/entry2-d1g35t.js',
                 javaScriptImports: [
-                    new JavaScriptImport('/assets/imported_file2.js', isLazy: false, asset: $importedFile2, addImplicitlyToImportMap: true),
-                    new JavaScriptImport('css_in_importmap', isLazy: false, asset: $importedCssInImportmap),
-                    new JavaScriptImport('/assets/styles/file2.css', isLazy: false, asset: $importedCss2, addImplicitlyToImportMap: true),
+                    new JavaScriptImport('/assets/imported_file2.js', asset: $importedFile2, isLazy: false, addImplicitlyToImportMap: true),
+                    new JavaScriptImport('css_in_importmap', asset: $importedCssInImportmap, isLazy: false),
+                    new JavaScriptImport('/assets/styles/file2.css', asset: $importedCss2, isLazy: false, addImplicitlyToImportMap: true),
                 ]
             ),
             new MappedAsset(
                 'entry3.js',
                 publicPath: '/assets/entry3-d1g35t.js',
                 javaScriptImports: [
-                    new JavaScriptImport('/assets/imported_file3.js', isLazy: false, asset: $importedFile3),
+                    new JavaScriptImport('/assets/imported_file3.js', asset: $importedFile3, isLazy: false),
                 ],
             ),
             $importedFile1,
@@ -342,7 +342,7 @@ public function getRawImportMapDataTests(): iterable
                 new MappedAsset(
                     'app.js',
                     publicPath: '/assets/app-d1g3st.js',
-                    javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset, addImplicitlyToImportMap: true)]
+                    javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false, addImplicitlyToImportMap: true)]
                 ),
                 $simpleAsset,
             ],
@@ -371,7 +371,7 @@ public function getRawImportMapDataTests(): iterable
                     'app.js',
                     sourcePath: '/assets/vendor/bootstrap.js',
                     publicPath: '/assets/vendor/bootstrap-d1g3st.js',
-                    javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset, addImplicitlyToImportMap: true)]
+                    javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false, addImplicitlyToImportMap: true)]
                 ),
                 $simpleAsset,
             ],
@@ -391,7 +391,7 @@ public function getRawImportMapDataTests(): iterable
             'imports_simple.js',
             publicPathWithoutDigest: '/assets/imports_simple.js',
             publicPath: '/assets/imports_simple-d1g3st.js',
-            javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset, addImplicitlyToImportMap: true)]
+            javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false, addImplicitlyToImportMap: true)]
         );
         yield 'it processes imports recursively' => [
             [
@@ -404,7 +404,7 @@ public function getRawImportMapDataTests(): iterable
                 new MappedAsset(
                     'app.js',
                     publicPath: '/assets/app-d1g3st.js',
-                    javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', isLazy: true, asset: $eagerImportsSimpleAsset, addImplicitlyToImportMap: true)]
+                    javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', asset: $eagerImportsSimpleAsset, isLazy: true, addImplicitlyToImportMap: true)]
                 ),
                 $eagerImportsSimpleAsset,
                 $simpleAsset,
@@ -440,7 +440,7 @@ public function getRawImportMapDataTests(): iterable
                 new MappedAsset(
                     'app.js',
                     publicPath: '/assets/app-d1g3st.js',
-                    javaScriptImports: [new JavaScriptImport('imports_simple', isLazy: true, asset: $eagerImportsSimpleAsset, addImplicitlyToImportMap: false)]
+                    javaScriptImports: [new JavaScriptImport('imports_simple', asset: $eagerImportsSimpleAsset, isLazy: true, addImplicitlyToImportMap: false)]
                 ),
                 $eagerImportsSimpleAsset,
                 $simpleAsset,
@@ -472,7 +472,7 @@ public function getRawImportMapDataTests(): iterable
                 new MappedAsset(
                     'app.js',
                     publicPath: '/assets/app-d1g3st.js',
-                    javaScriptImports: [new JavaScriptImport('simple', isLazy: false, asset: $simpleAsset)]
+                    javaScriptImports: [new JavaScriptImport('simple', asset: $simpleAsset, isLazy: false)]
                 ),
                 $simpleAsset,
             ],
@@ -609,7 +609,7 @@ public function getEagerEntrypointImportsTests(): iterable
             new MappedAsset(
                 'app.js',
                 publicPath: '/assets/app.js',
-                javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset)]
+                javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false)]
             ),
             ['/assets/simple.js'], // path is the key in the importmap
         ];
@@ -618,7 +618,7 @@ public function getEagerEntrypointImportsTests(): iterable
             new MappedAsset(
                 'app.js',
                 publicPath: '/assets/app.js',
-                javaScriptImports: [new JavaScriptImport('simple', isLazy: false, asset: $simpleAsset)]
+                javaScriptImports: [new JavaScriptImport('simple', asset: $simpleAsset, isLazy: false)]
             ),
             ['simple'], // path is the key in the importmap
         ];
@@ -627,21 +627,21 @@ public function getEagerEntrypointImportsTests(): iterable
             new MappedAsset(
                 'app.js',
                 publicPath: '/assets/app.js',
-                javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: true, asset: $simpleAsset)]
+                javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: true)]
             ),
             [],
         ];
 
         $importsSimpleAsset = new MappedAsset(
             'imports_simple.js',
             publicPathWithoutDigest: '/assets/imports_simple.js',
-            javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset)]
+            javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false)]
         );
         yield 'an entry follows through dependencies recursively' => [
             new MappedAsset(
                 'app.js',
                 publicPath: '/assets/app.js',
-                javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', isLazy: false, asset: $importsSimpleAsset)]
+                javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', asset: $importsSimpleAsset, isLazy: false)]
             ),
             ['/assets/imports_simple.js', '/assets/simple.js'],
         ];

src/Symfony/Component/AssetMapper/Tests/ImportMap/JavaScriptImportTest.php

@@ -20,7 +20,7 @@ class JavaScriptImportTest extends TestCase
     public function testBasicConstruction()
     {
         $asset = new MappedAsset('the-asset');
-        $import = new JavaScriptImport('the-import', true, $asset, true);
+        $import = new JavaScriptImport('the-import', $asset, true, true);
 
         $this->assertSame('the-import', $import->importName);
         $this->assertTrue($import->isLazy);

src/Symfony/Component/AssetMapper/Tests/MappedAssetTest.php

@@ -58,7 +58,7 @@ public function testAddJavaScriptImports()
         $mainAsset = new MappedAsset('file.js');
 
         $assetFoo = new MappedAsset('foo.js');
-        $javaScriptImport = new JavaScriptImport('/the_import', isLazy: true, asset: $assetFoo);
+        $javaScriptImport = new JavaScriptImport('/the_import', asset: $assetFoo, isLazy: true);
         $mainAsset->addJavaScriptImport($javaScriptImport);
 
         $this->assertSame([$javaScriptImport], $mainAsset->getJavaScriptImports());