Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 28, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions src/Symfony/Component/AssetMapper/Compiler/JavaScriptImportPathCompiler.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
));

Expand Down
5 changes: 5 additions & 0 deletions src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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;
}
}
8 changes: 3 additions & 5 deletions src/Symfony/Component/AssetMapper/ImportMap/ImportMapGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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;
Expand Down
12 changes: 4 additions & 8 deletions src/Symfony/Component/AssetMapper/ImportMap/JavaScriptImport.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}
}
}
16 changes: 11 additions & 5 deletions src/Symfony/Component/AssetMapper/Tests/Compiler/JavaScriptImportPathCompilerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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' => [
Expand Down Expand Up @@ -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' => [
Expand All @@ -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' => [
Expand Down
9 changes: 7 additions & 2 deletions src/Symfony/Component/AssetMapper/Tests/Factory/CachedMappedAssetFactoryTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -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);

Expand All @@ -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
Expand Down
36 changes: 18 additions & 18 deletions src/Symfony/Component/AssetMapper/Tests/ImportMap/ImportMapGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
],
Expand Down Expand Up @@ -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,
],
Expand All @@ -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' => [
[
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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,
],
Expand Down Expand Up @@ -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
];
Expand All @@ -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
];
Expand All @@ -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'],
];
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/AssetMapper/Tests/ImportMap/JavaScriptImportTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/Symfony/Component/AssetMapper/Tests/MappedAssetTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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());
Expand Down