-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[AssetMapper] Add cached asset factory #50286
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.
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
[AssetMapper] Add cached asset factory #50286
Conversation
@@ -25,20 +26,25 @@ class AssetMapperCompiler | |||
/** | |||
* @param iterable<AssetCompilerInterface> $assetCompilers | |||
*/ | |||
public function __construct(private iterable $assetCompilers) | |||
public function __construct(private iterable $assetCompilers, private ContainerInterface $container) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the ugliest part of this PR, and it's actually a small fix so that decorating the AssetMapper will work properly.
- It's super convenient for compilers to have access to the AssetMapper
- But, we get a circular reference: AssetMapper -> MappedAssetFactory -> AssetMapperCompiler -> AssetMapper
Is there a more elegant way to make just this one dependency lazy?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could inject a lazy proxy here, using the same kind of trick than used when using #[Autowire(lazy: AssetMapperInterface::class)]
for autowiring. But this would make the component much harder to use standalone if the circular dependency is between constructors (as the only way to instantiate the object graph would be through a lazy proxy)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But this would make the component much harder to use standalone
Thank you - that makes me want to keep this as it is :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about using a closure here, that'd return the mapper? that's a service_closure inthe DI world
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we can keep this as is TBH.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll fix this up shortly!
src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/CachedMappedAssetFactory.php
Outdated
Show resolved
Hide resolved
|
||
$resources[] = new FileResource($dependency->asset->getSourcePath()); | ||
} | ||
$configCache->write(serialize($mappedAsset), $resources); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the MappedAsset object contains AssetDependency objects that contain other MappedAsset objects, what happens when you serialize and unserialize those ?
Also, what is the impact of unserializing a MappedAsset with dependencies that will have MappedAsset objects not known by the AssetMapper internal cache (while potentially referring to the same assets) and also different from MappedAsset created for dependencies of other cached MappedAsset ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the MappedAsset object contains AssetDependency objects that contain other MappedAsset objects, what happens when you serialize and unserialize those ?
It seems to work fine, in practice. Both MappedAsset
and AssetDependency
are objects that hold scalars. For example, here is a dump of one situation (dumping the result of the unserialize
in the cached factory) in a test project:

What concerns do you have?
Also, what is the impact of unserializing a MappedAsset with dependencies that will have MappedAsset objects not known by the AssetMapper internal cache (while potentially referring to the same assets) and also different from MappedAsset created for dependencies of other cached MappedAsset ?
I think I understand what you're referring to. So, for example:
asset1.css
has a dependency onasset2.css
. When something asks for theasset1.css
asset, its cache is fresh, so we get backMappedAsset
instances from the cache for bothasset1.css
andasset2.css
.- Meanwhile,
asset3.css
also has a dependency onasset2.css
. When something asks for theasset3.css
asset, its cache is NOT fresh, so we get back FRESHMappedAsset
instances for bothasset3.css
andasset2.css
.
So, at this moment, there are two distinct asset2.css
MappedAsset
objects floating around the system. Their data is equal, but the objects are not identical. Other than being wasteful, I don't think it's a problem. MappedAsset
objects are "read-only" informational objects. Well, except for MappedAsset::addDependency()
. But, in practice, once a MappedAsset
is returned from AssetMapper
, it is intended to be read-only: its data is entirely set during the creation process (we could even "enforce" this by adding some flag to "freeze" the MappedAsset
and thus reject calling addDependency()
, but that might be overkill).
However, if we're concerned about this, we could, "refresh" the dependencies after fetching a cached MappedAsset
- e.g. loop over $mappedAsset->getDependencies()
... grab the ->logicalName
+ ->getSourcePath()
of each, call $this->createMappedAsset($logicalName, $sourcePath)
on each, then "replace" the MappedAsset
on each dependency.
tl;dr I don't see any real problem, but I could add extra code to be absolutely sure.
Should be rebased :) |
da1cc81
to
1536554
Compare
Rebased, reviewed again (by me) and tested again in my local app. Looks good 👍 |
src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Tests/Compiler/JavaScriptImportPathCompilerTest.php
Outdated
Show resolved
Hide resolved
0053044
to
d345b7e
Compare
This should be happy now - |
src/Symfony/Component/AssetMapper/Compiler/CssAssetUrlCompiler.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/Factory/MappedAssetFactory.php
Outdated
Show resolved
Hide resolved
d495722
to
5bb88ad
Compare
Updated - thanks for the suggestions :) |
Thank you @weaverryan. |
This PR was squashed before being merged into the 6.3 branch. Discussion ---------- [AssetMapper] Add cached asset factory | Q | A | ------------- | --- | Branch? | 6.3 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | None | License | MIT | Doc PR | Still TODO Hi! This is built on top of #50264. I CAN separate them if needed - sorry for the noise. tl;dr When using asset mapper, to load the page, we need to build the importmap & resolve a few paths, like `{{ asset('styles/app.css') }}`. Because asset mapper loads the contents and performs some regex on CSS and JS files, this can slow down the page in dev (in prod, `asset-map:compile` removes all overhead). This PR fixes that (I can see a big improvement in my test app). We "cache" the `MappedAsset` objects via the config cache system so that if an underlying file changes (e.g. `assets/styles/app.css`), we only need to rebuild that ONE `MappedAsset` when the page is loading. Cheers! Commits ------- ce77ed8 [AssetMapper] Add cached asset factory
5bb88ad
to
ce77ed8
Compare
Hi!
This is built on top of #50264. I CAN separate them if needed - sorry for the noise.
tl;dr When using asset mapper, to load the page, we need to build the importmap & resolve a few paths, like
{{ asset('styles/app.css') }}
. Because asset mapper loads the contents and performs some regex on CSS and JS files, this can slow down the page in dev (in prod,asset-map:compile
removes all overhead).This PR fixes that (I can see a big improvement in my test app). We "cache" the
MappedAsset
objects via the config cache system so that if an underlying file changes (e.g.assets/styles/app.css
), we only need to rebuild that ONEMappedAsset
when the page is loading.Cheers!