Skip to content

Commit 6fad9a4

Browse files
fabpotfabpot
committed
bug #52331 [AssetMapper] Fix file deleting errors & remove nullable MappedAsset 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
2 parents eaff34a + f1708aa commit 6fad9a4

9 files changed

+56
-42
lines changed

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

+6-2
Original file line numberDiff line numberDiff line change
@@ -61,15 +61,19 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
6161
$dependentAsset = $this->findAssetForRelativeImport($importedModule, $asset, $assetMapper);
6262
}
6363

64+
if (!$dependentAsset) {
65+
return $fullImportString;
66+
}
67+
6468
// List as a JavaScript import.
6569
// This will cause the asset to be included in the importmap (for relative imports)
6670
// and will be used to generate the preloads in the importmap.
6771
$isLazy = str_contains($fullImportString, 'import(');
68-
$addToImportMap = $isRelativeImport && $dependentAsset;
72+
$addToImportMap = $isRelativeImport;
6973
$asset->addJavaScriptImport(new JavaScriptImport(
7074
$addToImportMap ? $dependentAsset->publicPathWithoutDigest : $importedModule,
71-
$isLazy,
7275
$dependentAsset,
76+
$isLazy,
7377
$addToImportMap,
7478
));
7579

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

+5
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
use Symfony\Component\AssetMapper\MappedAsset;
1515
use Symfony\Component\Config\ConfigCache;
1616
use Symfony\Component\Config\Resource\DirectoryResource;
17+
use Symfony\Component\Config\Resource\FileExistenceResource;
1718
use Symfony\Component\Config\Resource\FileResource;
1819
use Symfony\Component\Config\Resource\ResourceInterface;
1920

@@ -67,6 +68,10 @@ private function collectResourcesFromAsset(MappedAsset $mappedAsset): array
6768
$resources = array_merge($resources, $this->collectResourcesFromAsset($assetDependency));
6869
}
6970

71+
foreach ($mappedAsset->getJavaScriptImports() as $import) {
72+
$resources[] = new FileExistenceResource($import->asset->sourcePath);
73+
}
74+
7075
return $resources;
7176
}
7277
}

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

+3-5
Original file line numberDiff line numberDiff line change
@@ -178,7 +178,7 @@ private function addImplicitEntries(ImportMapEntry $entry, array $currentImportE
178178
}
179179

180180
// check if this import requires an automatic importmap entry
181-
if ($javaScriptImport->addImplicitlyToImportMap && $javaScriptImport->asset) {
181+
if ($javaScriptImport->addImplicitlyToImportMap) {
182182
$nextEntry = ImportMapEntry::createLocal(
183183
$importName,
184184
ImportMapType::tryFrom($javaScriptImport->asset->publicExtension) ?: ImportMapType::JS,
@@ -226,10 +226,8 @@ private function findEagerImports(MappedAsset $asset): array
226226

227227
$dependencies[] = $javaScriptImport->importName;
228228

229-
// the import is for a MappedAsset? Follow its imports!
230-
if ($javaScriptImport->asset) {
231-
$dependencies = array_merge($dependencies, $this->findEagerImports($javaScriptImport->asset));
232-
}
229+
// Follow its imports!
230+
$dependencies = array_merge($dependencies, $this->findEagerImports($javaScriptImport->asset));
233231
}
234232

235233
return $dependencies;

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

+4-8
Original file line numberDiff line numberDiff line change
@@ -19,19 +19,15 @@
1919
final class JavaScriptImport
2020
{
2121
/**
22-
* @param string $importName The name of the import needed in the importmap, e.g. "/foo.js" or "react"
23-
* @param bool $isLazy Whether this import was lazy or eager
24-
* @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
25-
* @param bool $addImplicitlyToImportMap Whether this import should be added to the importmap automatically
22+
* @param string $importName The name of the import needed in the importmap, e.g. "/foo.js" or "react"
23+
* @param MappedAsset $asset The asset that was imported
24+
* @param bool $addImplicitlyToImportMap Whether this import should be added to the importmap automatically
2625
*/
2726
public function __construct(
2827
public readonly string $importName,
28+
public readonly MappedAsset $asset,
2929
public readonly bool $isLazy = false,
30-
public readonly ?MappedAsset $asset = null,
3130
public bool $addImplicitlyToImportMap = false,
3231
) {
33-
if (null === $asset && $addImplicitlyToImportMap) {
34-
throw new \LogicException(sprintf('The "%s" import cannot be automatically added to the importmap without an asset.', $this->importName));
35-
}
3632
}
3733
}

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

+11-5
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,7 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
7272
$this->assertSame($input, $compiler->compile($input, $asset, $assetMapper));
7373
$actualImports = [];
7474
foreach ($asset->getJavaScriptImports() as $import) {
75-
$actualImports[$import->importName] = ['lazy' => $import->isLazy, 'asset' => $import->asset?->logicalPath, 'add' => $import->addImplicitlyToImportMap];
75+
$actualImports[$import->importName] = ['lazy' => $import->isLazy, 'asset' => $import->asset->logicalPath, 'add' => $import->addImplicitlyToImportMap];
7676
}
7777

7878
$this->assertEquals($expectedJavaScriptImports, $actualImports);
@@ -172,9 +172,10 @@ public static function provideCompileTests(): iterable
172172
'expectedJavaScriptImports' => ['/assets/styles.css' => ['lazy' => false, 'asset' => 'styles.css', 'add' => true]],
173173
];
174174

175-
yield 'importing_non_existent_file_without_strict_mode_is_ignored_still_listed_as_an_import' => [
175+
yield 'importing_non_existent_file_without_strict_mode_is_ignored_and_no_import_added' => [
176+
'sourceLogicalName' => 'app.js',
176177
'input' => "import './non-existent.js';",
177-
'expectedJavaScriptImports' => ['./non-existent.js' => ['lazy' => false, 'asset' => null, 'add' => false]],
178+
'expectedJavaScriptImports' => [],
178179
];
179180

180181
yield 'single_line_comment_at_start_ignored' => [
@@ -262,7 +263,7 @@ public static function provideCompileTests(): iterable
262263

263264
yield 'bare_import_not_in_importmap' => [
264265
'input' => 'import "some_module";',
265-
'expectedJavaScriptImports' => ['some_module' => ['lazy' => false, 'asset' => null, 'add' => false]],
266+
'expectedJavaScriptImports' => [],
266267
];
267268

268269
yield 'bare_import_in_importmap_with_local_asset' => [
@@ -275,9 +276,14 @@ public static function provideCompileTests(): iterable
275276
'expectedJavaScriptImports' => ['module_in_importmap_remote' => ['lazy' => false, 'asset' => 'module_in_importmap_remote.js', 'add' => false]],
276277
];
277278

279+
<<<<<<< HEAD
278280
yield 'absolute_import_added_as_dependency_only' => [
281+
=======
282+
yield 'absolute_import_ignored_and_no_dependency_added' => [
283+
'sourceLogicalName' => 'app.js',
284+
>>>>>>> a79f543f8f ([AssetMapper] Fix file deleting errors & remove nullable MappedAsset on JS import)
279285
'input' => 'import "https://example.com/module.js";',
280-
'expectedJavaScriptImports' => ['https://example.com/module.js' => ['lazy' => false, 'asset' => null, 'add' => false]],
286+
'expectedJavaScriptImports' => [],
281287
];
282288

283289
yield 'bare_import_with_minimal_spaces' => [

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

+7-2
Original file line numberDiff line numberDiff line change
@@ -14,9 +14,11 @@
1414
use PHPUnit\Framework\TestCase;
1515
use Symfony\Component\AssetMapper\Factory\CachedMappedAssetFactory;
1616
use Symfony\Component\AssetMapper\Factory\MappedAssetFactoryInterface;
17+
use Symfony\Component\AssetMapper\ImportMap\JavaScriptImport;
1718
use Symfony\Component\AssetMapper\MappedAsset;
1819
use Symfony\Component\Config\ConfigCache;
1920
use Symfony\Component\Config\Resource\DirectoryResource;
21+
use Symfony\Component\Config\Resource\FileExistenceResource;
2022
use Symfony\Component\Config\Resource\FileResource;
2123
use Symfony\Component\Filesystem\Filesystem;
2224

@@ -89,9 +91,11 @@ public function testAssetConfigCacheResourceContainsDependencies()
8991
$mappedAsset = new MappedAsset('file1.css', $sourcePath, content: 'cached content');
9092

9193
$dependentOnContentAsset = new MappedAsset('file3.css', realpath(__DIR__.'/../Fixtures/dir2/file3.css'));
92-
9394
$deeplyNestedAsset = new MappedAsset('file4.js', realpath(__DIR__.'/../Fixtures/dir2/file4.js'));
9495

96+
$file6Asset = new MappedAsset('file6.js', realpath(__DIR__.'/../Fixtures/dir2/subdir/file6.js'));
97+
$deeplyNestedAsset->addJavaScriptImport(new JavaScriptImport('file6', asset: $file6Asset));
98+
9599
$dependentOnContentAsset->addDependency($deeplyNestedAsset);
96100
$mappedAsset->addDependency($dependentOnContentAsset);
97101

@@ -112,14 +116,15 @@ public function testAssetConfigCacheResourceContainsDependencies()
112116
$cachedFactory->createMappedAsset('file1.css', $sourcePath);
113117

114118
$configCacheMetadata = $this->loadConfigCacheMetadataFor($mappedAsset);
115-
$this->assertCount(5, $configCacheMetadata);
119+
$this->assertCount(6, $configCacheMetadata);
116120
$this->assertInstanceOf(FileResource::class, $configCacheMetadata[0]);
117121
$this->assertInstanceOf(DirectoryResource::class, $configCacheMetadata[1]);
118122
$this->assertInstanceOf(FileResource::class, $configCacheMetadata[2]);
119123
$this->assertSame(realpath(__DIR__.'/../Fixtures/importmap.php'), $configCacheMetadata[0]->getResource());
120124
$this->assertSame($mappedAsset->sourcePath, $configCacheMetadata[2]->getResource());
121125
$this->assertSame($dependentOnContentAsset->sourcePath, $configCacheMetadata[3]->getResource());
122126
$this->assertSame($deeplyNestedAsset->sourcePath, $configCacheMetadata[4]->getResource());
127+
$this->assertInstanceOf(FileExistenceResource::class, $configCacheMetadata[5]);
123128
}
124129

125130
private function loadConfigCacheMetadataFor(MappedAsset $mappedAsset): array

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

+18-18
Original file line numberDiff line numberDiff line change
@@ -139,25 +139,25 @@ public function testGetImportMapData()
139139
'entry1.js',
140140
publicPath: '/assets/entry1-d1g35t.js',
141141
javaScriptImports: [
142-
new JavaScriptImport('/assets/imported_file1.js', isLazy: false, asset: $importedFile1, addImplicitlyToImportMap: true),
143-
new JavaScriptImport('/assets/styles/file1.css', isLazy: false, asset: $importedCss1, addImplicitlyToImportMap: true),
144-
new JavaScriptImport('normal_js_file', isLazy: false, asset: $normalJsFile),
142+
new JavaScriptImport('/assets/imported_file1.js', asset: $importedFile1, isLazy: false, addImplicitlyToImportMap: true),
143+
new JavaScriptImport('/assets/styles/file1.css', asset: $importedCss1, isLazy: false, addImplicitlyToImportMap: true),
144+
new JavaScriptImport('normal_js_file', asset: $normalJsFile, isLazy: false),
145145
]
146146
),
147147
new MappedAsset(
148148
'entry2.js',
149149
publicPath: '/assets/entry2-d1g35t.js',
150150
javaScriptImports: [
151-
new JavaScriptImport('/assets/imported_file2.js', isLazy: false, asset: $importedFile2, addImplicitlyToImportMap: true),
152-
new JavaScriptImport('css_in_importmap', isLazy: false, asset: $importedCssInImportmap),
153-
new JavaScriptImport('/assets/styles/file2.css', isLazy: false, asset: $importedCss2, addImplicitlyToImportMap: true),
151+
new JavaScriptImport('/assets/imported_file2.js', asset: $importedFile2, isLazy: false, addImplicitlyToImportMap: true),
152+
new JavaScriptImport('css_in_importmap', asset: $importedCssInImportmap, isLazy: false),
153+
new JavaScriptImport('/assets/styles/file2.css', asset: $importedCss2, isLazy: false, addImplicitlyToImportMap: true),
154154
]
155155
),
156156
new MappedAsset(
157157
'entry3.js',
158158
publicPath: '/assets/entry3-d1g35t.js',
159159
javaScriptImports: [
160-
new JavaScriptImport('/assets/imported_file3.js', isLazy: false, asset: $importedFile3),
160+
new JavaScriptImport('/assets/imported_file3.js', asset: $importedFile3, isLazy: false),
161161
],
162162
),
163163
$importedFile1,
@@ -342,7 +342,7 @@ public function getRawImportMapDataTests(): iterable
342342
new MappedAsset(
343343
'app.js',
344344
publicPath: '/assets/app-d1g3st.js',
345-
javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset, addImplicitlyToImportMap: true)]
345+
javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false, addImplicitlyToImportMap: true)]
346346
),
347347
$simpleAsset,
348348
],
@@ -371,7 +371,7 @@ public function getRawImportMapDataTests(): iterable
371371
'app.js',
372372
sourcePath: '/assets/vendor/bootstrap.js',
373373
publicPath: '/assets/vendor/bootstrap-d1g3st.js',
374-
javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset, addImplicitlyToImportMap: true)]
374+
javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false, addImplicitlyToImportMap: true)]
375375
),
376376
$simpleAsset,
377377
],
@@ -391,7 +391,7 @@ public function getRawImportMapDataTests(): iterable
391391
'imports_simple.js',
392392
publicPathWithoutDigest: '/assets/imports_simple.js',
393393
publicPath: '/assets/imports_simple-d1g3st.js',
394-
javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset, addImplicitlyToImportMap: true)]
394+
javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false, addImplicitlyToImportMap: true)]
395395
);
396396
yield 'it processes imports recursively' => [
397397
[
@@ -404,7 +404,7 @@ public function getRawImportMapDataTests(): iterable
404404
new MappedAsset(
405405
'app.js',
406406
publicPath: '/assets/app-d1g3st.js',
407-
javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', isLazy: true, asset: $eagerImportsSimpleAsset, addImplicitlyToImportMap: true)]
407+
javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', asset: $eagerImportsSimpleAsset, isLazy: true, addImplicitlyToImportMap: true)]
408408
),
409409
$eagerImportsSimpleAsset,
410410
$simpleAsset,
@@ -440,7 +440,7 @@ public function getRawImportMapDataTests(): iterable
440440
new MappedAsset(
441441
'app.js',
442442
publicPath: '/assets/app-d1g3st.js',
443-
javaScriptImports: [new JavaScriptImport('imports_simple', isLazy: true, asset: $eagerImportsSimpleAsset, addImplicitlyToImportMap: false)]
443+
javaScriptImports: [new JavaScriptImport('imports_simple', asset: $eagerImportsSimpleAsset, isLazy: true, addImplicitlyToImportMap: false)]
444444
),
445445
$eagerImportsSimpleAsset,
446446
$simpleAsset,
@@ -472,7 +472,7 @@ public function getRawImportMapDataTests(): iterable
472472
new MappedAsset(
473473
'app.js',
474474
publicPath: '/assets/app-d1g3st.js',
475-
javaScriptImports: [new JavaScriptImport('simple', isLazy: false, asset: $simpleAsset)]
475+
javaScriptImports: [new JavaScriptImport('simple', asset: $simpleAsset, isLazy: false)]
476476
),
477477
$simpleAsset,
478478
],
@@ -609,7 +609,7 @@ public function getEagerEntrypointImportsTests(): iterable
609609
new MappedAsset(
610610
'app.js',
611611
publicPath: '/assets/app.js',
612-
javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset)]
612+
javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false)]
613613
),
614614
['/assets/simple.js'], // path is the key in the importmap
615615
];
@@ -618,7 +618,7 @@ public function getEagerEntrypointImportsTests(): iterable
618618
new MappedAsset(
619619
'app.js',
620620
publicPath: '/assets/app.js',
621-
javaScriptImports: [new JavaScriptImport('simple', isLazy: false, asset: $simpleAsset)]
621+
javaScriptImports: [new JavaScriptImport('simple', asset: $simpleAsset, isLazy: false)]
622622
),
623623
['simple'], // path is the key in the importmap
624624
];
@@ -627,21 +627,21 @@ public function getEagerEntrypointImportsTests(): iterable
627627
new MappedAsset(
628628
'app.js',
629629
publicPath: '/assets/app.js',
630-
javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: true, asset: $simpleAsset)]
630+
javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: true)]
631631
),
632632
[],
633633
];
634634

635635
$importsSimpleAsset = new MappedAsset(
636636
'imports_simple.js',
637637
publicPathWithoutDigest: '/assets/imports_simple.js',
638-
javaScriptImports: [new JavaScriptImport('/assets/simple.js', isLazy: false, asset: $simpleAsset)]
638+
javaScriptImports: [new JavaScriptImport('/assets/simple.js', asset: $simpleAsset, isLazy: false)]
639639
);
640640
yield 'an entry follows through dependencies recursively' => [
641641
new MappedAsset(
642642
'app.js',
643643
publicPath: '/assets/app.js',
644-
javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', isLazy: false, asset: $importsSimpleAsset)]
644+
javaScriptImports: [new JavaScriptImport('/assets/imports_simple.js', asset: $importsSimpleAsset, isLazy: false)]
645645
),
646646
['/assets/imports_simple.js', '/assets/simple.js'],
647647
];

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ class JavaScriptImportTest extends TestCase
2020
public function testBasicConstruction()
2121
{
2222
$asset = new MappedAsset('the-asset');
23-
$import = new JavaScriptImport('the-import', true, $asset, true);
23+
$import = new JavaScriptImport('the-import', $asset, true, true);
2424

2525
$this->assertSame('the-import', $import->importName);
2626
$this->assertTrue($import->isLazy);

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

+1-1
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public function testAddJavaScriptImports()
5858
$mainAsset = new MappedAsset('file.js');
5959

6060
$assetFoo = new MappedAsset('foo.js');
61-
$javaScriptImport = new JavaScriptImport('/the_import', isLazy: true, asset: $assetFoo);
61+
$javaScriptImport = new JavaScriptImport('/the_import', asset: $assetFoo, isLazy: true);
6262
$mainAsset->addJavaScriptImport($javaScriptImport);
6363

6464
$this->assertSame([$javaScriptImport], $mainAsset->getJavaScriptImports());

0 commit comments

Comments
 (0)