8000 [AssetMapper] Avoid storing & caching MappedAsset objects · symfony/symfony@caf7fcb · GitHub
[go: up one dir, main page]
Skip to content

Commit caf7fcb

Browse files
weaverryanweaverryan
committed
[AssetMapper] Avoid storing & caching MappedAsset objects
The problem is that these could get out of date, allowing asset Foo to have a JavaScript import for the MappedAsset for Bar, but it is out of date because Bar has since changed.
1 parent 5aaa534 commit caf7fcb

File tree

10 files changed

+89
-59
lines changed

10 files changed

+89
-59
lines changed

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -72,7 +72,8 @@ public function compile(string $content, MappedAsset $asset, AssetMapperInterfac
7272
$addToImportMap = $isRelativeImport;
7373
$asset->addJavaScriptImport(new JavaScriptImport(
7474
$addToImportMap ? $dependentAsset->publicPathWithoutDigest : $importedModule,
75-
$dependentAsset,
75+
$dependentAsset->logicalPath,
76+
$dependentAsset->sourcePath,
7677
$isLazy,
7778
$addToImportMap,
7879
));

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ private function collectResourcesFromAsset(MappedAsset $mappedAsset): array
6969
}
7070

7171
foreach ($mappedAsset->getJavaScriptImports() as $import) {
72-
$resources[] = new FileExistenceResource($import->asset->sourcePath);
72+
$resources[] = new FileExistenceResource($import->assetSourcePath);
7373
}
7474

7575
return $resources;

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

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -179,10 +179,16 @@ private function addImplicitEntries(ImportMapEntry $entry, array $currentImportE
179179

180180
// check if this import requires an automatic importmap entry
181181
if ($javaScriptImport->addImplicitlyToImportMap) {
182+
$importedAsset = $this->assetMapper->getAsset($javaScriptImport->assetLogicalPath);
183+
if (!$importedAsset) {
184+
// should not happen at this point, unless something added a bogus JavaScriptImport to this asset
185+
throw new LogicException(sprintf('Cannot find imported JavaScript asset "%s" in asset mapper.', $javaScriptImport->assetLogicalPath));
186+
}
187+
182188
$nextEntry = ImportMapEntry::createLocal(
183189
$importName,
184-
ImportMapType::tryFrom($javaScriptImport->asset->publicExtension) ?: ImportMapType::JS,
185-
$javaScriptImport->asset->logicalPath,
190+
ImportMapType::tryFrom($importedAsset->publicExtension) ?: ImportMapType::JS,
191+
$importedAsset->logicalPath,
186192
false,
187193
);
188194

@@ -223,7 +229,12 @@ private function findEagerImports(MappedAsset $asset): array
223229
$dependencies[] = $javaScriptImport->importName;
224230

225231
// Follow its imports!
226-
$dependencies = array_merge($dependencies, $this->findEagerImports($javaScriptImport->asset));
232+
$nextAsset = $this->assetMapper->getAsset($javaScriptImport->assetLogicalPath);
233+
if (!$nextAsset) {
234+
// should not happen at this point, unless something added a bogus JavaScriptImport to this asset
235+
throw new LogicException(sprintf('Cannot find imported JavaScript asset "%s" in asset mapper.', $javaScriptImport->asse 9E88 tLogicalPath));
236+
}
237+
$dependencies = array_merge($dependencies, $this->findEagerImports($nextAsset));
227238
}
228239

229240
return $dependencies;

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -11,21 +11,20 @@
1111

1212
namespace Symfony\Component\AssetMapper\ImportMap;
1313

14-
use Symfony\Component\AssetMapper\MappedAsset;
15-
1614
/**
1715
* Represents a module that was imported by a JavaScript file.
1816
*/
1917
final class JavaScriptImport
2018
{
2119
/**
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
20+
* @param string $importName The name of the import needed in the importmap, e.g. "/foo.js" or "react"
21+
* @param string $assetLogicalPath logical path to the mapped ass that was imported
22+
* @param bool $addImplicitlyToImportMap Whether this import should be added to the importmap automatically
2523
*/
2624
public function __construct(
2725
public readonly string $importName,
28-
public readonly MappedAsset $asset,
26+
public readonly string $assetLogicalPath,
27+
public readonly string $assetSourcePath,
2928
public readonly bool $isLazy = false,
3029
public bool $addImplicitlyToImportMap = false,
3130
) {

‎src/Symfony/Component/AssetMapper/MappedAsset.php

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,8 @@ public function __construct(
9494
}
9595

9696
/**
97+
* Assets that the content of this asset depends on - for internal caching.
98+
*
9799
* @return MappedAsset[]
98100
*/
99101
public function getDependencies(): array

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

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
5858
->method('getAsset')
5959
->willReturnCallback(function ($path) {
6060
return match ($path) {
61-
'module_in_importmap_local_asset.js' => new MappedAsset('module_in_importmap_local_asset.js', publicPathWithoutDigest: '/assets/module_in_importmap_local_asset.js'),
61+
'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'),
6262
default => null,
6363
};
6464
});
@@ -67,11 +67,11 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
6767
->method('getAssetFromSourcePath')
6868
->willReturnCallback(function ($path) {
6969
return match ($path) {
70-
'/project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
71-
'/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
72-
'/project/assets/styles.css' => new MappedAsset('styles.css', publicPathWithoutDigest: '/assets/styles.css'),
73-
'/project/assets/vendor/module_in_importmap_remote.js' => new MappedAsset('module_in_importmap_remote.js', publicPathWithoutDigest: '/assets/module_in_importmap_remote.js'),
74-
'/project/assets/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
70+
'/project/assets/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
71+
'/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
72+
'/project/assets/styles.css' => new MappedAsset('styles.css', '/can/be/anything.js', publicPathWithoutDigest: '/assets/styles.css'),
73+
'/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'),
74+
'/project/assets/vendor/@popperjs/core.js' => new MappedAsset('assets/vendor/@popperjs/core.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/@popperjs/core.js'),
7575
default => null,
7676
};
7777
});
@@ -81,7 +81,7 @@ public function testCompileFindsCorrectImports(string $input, array $expectedJav
8181
$this->assertSame($input, $compiler->compile($input, $asset, $assetMapper));
8282
$actualImports = [];
8383
foreach ($asset->getJavaScriptImports() as $import) {
84-
$actualImports[$import->importName] = ['lazy' => $import->isLazy, 'asset' => $import->asset->logicalPath, 'add' => $import->addImplicitlyToImportMap];
84+
$actualImports[$import->importName] = ['lazy' => $import->isLazy, 'asset' => $import->assetLogicalPath, 'add' => $import->addImplicitlyToImportMap];
8585
}
8686

8787
$this->assertEquals($expectedJavaScriptImports, $actualImports);
@@ -304,9 +304,9 @@ public function testCompileFindsRelativePathsViaSourcePath()
304304
->method('getAssetFromSourcePath')
305305
->willReturnCallback(function ($path) {
306306
return match ($path) {
307-
'/project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
308-
'/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
309-
'/project/root_asset.js' => new MappedAsset('root_asset.js', publicPathWithoutDigest: '/assets/root_asset.js'),
307+
'/project/assets/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
308+
'/project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
309+
'/project/root_asset.js' => new MappedAsset('root_asset.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/root_asset.js'),
310310
default => throw new \RuntimeException(sprintf('Unexpected source path "%s"', $path)),
311311
};
312312
});
@@ -320,9 +320,9 @@ public function testCompileFindsRelativePathsViaSourcePath()
320320
$compiler = new JavaScriptImportPathCompiler($this->createMock(ImportMapConfigReader::class));
321321
$compiler->compile($input, $inputAsset, $assetMapper);
322322
$this->assertCount(3, $inputAsset->getJavaScriptImports());
323-
$this->assertSame('other.js', $inputAsset->getJavaScriptImports()[0]->asset->logicalPath);
324-
$this->assertSame('subdir/foo.js', $inputAsset->getJavaScriptImports()[1]->asset->logicalPath);
325-
$this->assertSame('root_asset.js', $inputAsset->getJavaScriptImports()[2]->asset->logicalPath);
323+
$this->assertSame('other.js', $inputAsset->getJavaScriptImports()[0]->assetLogicalPath);
324+
$this->assertSame('subdir/foo.js', $inputAsset->getJavaScriptImports()[1]->assetLogicalPath);
325+
$this->assertSame('root_asset.js', $inputAsset->getJavaScriptImports()[2]->assetLogicalPath);
326326
}
327327

328328
public function testCompileFindsRelativePathsWithWindowsPathsViaSourcePath()
@@ -337,9 +337,9 @@ public function testCompileFindsRelativePathsWithWindowsPathsViaSourcePath()
337337
->method('getAssetFromSourcePath')
338338
->willReturnCallback(function ($path) {
339339
return match ($path) {
340-
'C://project/assets/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
341-
'C://project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
342-
'C://project/root_asset.js' => new MappedAsset('root_asset.js', publicPathWithoutDigest: '/assets/root_asset.js'),
340+
'C://project/assets/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
341+
'C://project/assets/subdir/foo.js' => new MappedAsset('subdir/foo.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/subdir/foo.js'),
342+
'C://project/root_asset.js' => new MappedAsset('root_asset.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/root_asset.js'),
343343
default => throw new \RuntimeException(sprintf('Unexpected source path "%s"', $path)),
344344
};
345345
});
@@ -366,7 +366,7 @@ public function testImportPathsCanUpdateForDifferentPublicPath(string $input, st
366366
$asset = new MappedAsset('app.js', '/path/to/assets/app.js', publicPathWithoutDigest: $inputAssetPublicPath);
367367

368368
$assetMapper = $this->createMock(AssetMapperInterface::class);
369-
$importedAsset = new MappedAsset('anything', publicPathWithoutDigest: $importedPublicPath);
369+
$importedAsset = new MappedAsset('anything', '/can/be/anything.js', publicPathWithoutDigest: $importedPublicPath);
370370
$assetMapper->expects($this->once())
371371
->method('getAssetFromSourcePath')
372372
->willReturn($importedAsset);
@@ -436,7 +436,7 @@ public function testCompileHandlesCircularRelativeAssets()
436436
$input = 'import "./other.js";';
437437
$compiler->compile($input, $appAsset, $assetMapper);
438438
$this->assertCount(1, $appAsset->getJavaScriptImports());
439-
$this->assertSame($otherAsset, $appAsset->getJavaScriptImports()[0]->asset);
439+
$this->assertSame($otherAsset->logicalPath, $appAsset->getJavaScriptImports()[0]->assetLogicalPath);
440440
}
441441

442442
public function testCompileHandlesCircularBareImportAssets()
@@ -464,7 +464,7 @@ public function testCompileHandlesCircularBareImportAssets()
464464
$input = 'import "@popperjs/core";';
465465
$compiler->compile($input, $bootstrapAsset, $assetMapper);
466466
$this->assertCount(1, $bootstrapAsset->getJavaScriptImports());
467-
$this->assertSame($popperAsset, $bootstrapAsset->getJavaScriptImports()[0]->asset);
467+
$this->assertSame($popperAsset->logicalPath, $bootstrapAsset->getJavaScriptImports()[0]->assetLogicalPath);
468468
}
469469

470470
/**
@@ -490,7 +490,7 @@ public function testMissingImportMode(string $sourceLogicalName, string $input,
490490
->method('getAssetFromSourcePath')
491491
->willReturnCallback(function ($sourcePath) {
492492
return match ($sourcePath) {
493-
'/path/to/other.js' => new MappedAsset('other.js', publicPathWithoutDigest: '/assets/other.js'),
493+
'/path/to/other.js' => new MappedAsset('other.js', '/can/be/anything.js', publicPathWithoutDigest: '/assets/other.js'),
494494
default => null,
495495
};
496496
}

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ public function testAssetConfigCacheResourceContainsDependencies()
9494
$deeplyNestedAsset = new MappedAsset('file4.js', realpath(__DIR__.'/../Fixtures/dir2/file4.js'));
9595

9696
$file6Asset = new MappedAsset('file6.js', realpath(__DIR__.'/../Fixtures/dir2/subdir/file6.js'));
97-
$deeplyNestedAsset->addJavaScriptImport(new JavaScriptImport('file6', asset: $file6Asset));
97+
$deeplyNestedAsset->addJavaScriptImport(new JavaScriptImport('file6', assetLogicalPath: $file6Asset->logicalPath, assetSourcePath: $file6Asset->sourcePath));
9898

9999
$dependentOnContentAsset->addDependency($deeplyNestedAsset);
100100
$mappedAsset->addDependency($dependentOnContentAsset);

0 commit comments

Comments
 (0)
0