Skip to content

Commit

Permalink
bug #52523 [AssetMapper] avoid caching MappedAsset inside JavaScript …
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
nicolas-grekas committed Nov 10, 2023
2 parents fa4726f + bb5ef88 commit a647f55
Show file tree
Hide file tree
Showing 10 changed files with 87 additions and 59 deletions.
Expand Up @@ -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,
));
Expand Down
Expand Up @@ -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;
Expand Down
15 changes: 12 additions & 3 deletions src/Symfony/Component/AssetMapper/ImportMap/ImportMapGenerator.php
Expand Up @@ -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,
);

Expand Down Expand Up @@ -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;
Expand Down
11 changes: 5 additions & 6 deletions src/Symfony/Component/AssetMapper/ImportMap/JavaScriptImport.php
Expand Up @@ -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,
) {
Expand Down
2 changes: 2 additions & 0 deletions src/Symfony/Component/AssetMapper/MappedAsset.php
Expand Up @@ -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
Expand Down
Expand Up @@ -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,
};
});
Expand All @@ -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,
};
});
Expand All @@ -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);
Expand Down Expand Up @@ -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)),
};
});
Expand All @@ -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()
Expand All @@ -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)),
};
});
Expand All @@ -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);
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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);
}

/**
Expand All @@ -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,
};
}
Expand Down
Expand Up @@ -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);
Expand Down

0 comments on commit a647f55

Please sign in to comment.