Skip to content

[AssetMapper][FrameworkBundle] Add cache warmer for AssetMapper compilation #59089

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

Conversation

alexandre-daubois
Copy link
Member

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #59072
License MIT

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe we need two cache warmers: one for debug and one for no-debug (prod)?
the one for debug should remove files, so that the assets can be served dynamically

* @author Ryan Weaver <ryan@symfonycasts.com>
* @author Alexandre Daubois <alex.daubois@gmail.com>
*/
final class AssetMapperCompileCacheWarmer implements CacheWarmerInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AssetMapperCacheWarmer (same in the changelog)

@alexandre-daubois
Copy link
Member Author

Should we keep one warmer but inject kernel.debug to determine if files should be removed? Without contraindication, this would be easier to maintain

@alexandre-daubois alexandre-daubois force-pushed the asset-mapper-cache-warmer branch from e6c0090 to 5587c5c Compare December 4, 2024 14:06
@nicolas-grekas
Copy link
Member

This would mean creating (or even keeping) services for nothing. Not sure it'd be easier to maintain also.

@alexandre-daubois
Copy link
Member Author

Oh, by "the one for debug should remove files", you mean "remove files and nothing else"? If yes, indeed two warmers are more relevant, I'l do the change.

@nicolas-grekas
Copy link
Member

Like in #59073 yes

@alexandre-daubois alexandre-daubois force-pushed the asset-mapper-cache-warmer branch from 5587c5c to 9657b3e Compare December 5, 2024 07:41
@alexandre-daubois
Copy link
Member Author

Second cache warmer added!

* @author Ryan Weaver <ryan@symfonycasts.com>
* @author Alexandre Daubois <alex.daubois@gmail.com>
*/
final class AssetMapperCacheWarmer implements CacheWarmerInterface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This warmer seems to have a lot in common with the CompileCommand (logically 😅 ) ... should we not try to keep that code in one place only ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be better indeed! However, the command outputs a lot of useful info through console's OutputInterface (which the warmer doesn't have access to). We could create a service containing the logic, with an optional OutputInterface. Not sure it worth it given the warmer in pretty concise?

$entrypointFiles[$entrypointName] = $path;
}

$this->eventDispatcher?->dispatch(new PreAssetsCompileEvent(new NullOutput()));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will all services / listeners be available during Cache Warm ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes! You'll be able to register your event listeners just as you do normally 🙂

Comment on lines +46 to +47
$this->compiledConfigReader->removeConfig(AssetMapper::MANIFEST_FILE_NAME);
$this->compiledConfigReader->removeConfig(ImportMapGenerator::IMPORT_MAP_CACHE_FILENAME);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can anyone disable this cache warmer ? As it destroys data and people have many reasons to separate cache warm and asset build, this feels more than required

(sorry if i missread something here)

Copy link
Member Author

@alexandre-daubois alexandre-daubois Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you have better knowledge of this component than me 🙂 How would you like to opt-in/out from this warmer?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would make it optional to start, so that cache:clear does not force all assets (images, videos, idk.. to be read in memory).

And i realize this is the reason for this architeccture at first. I remember asking about the config files beeing outside, and i think the answer was to make this independant from the php code.

So, we must have a way (even, the default way i'd say for BC reasons) to build a Symfony app without deleting all its asset config.

@smnandre
Copy link
Member

Ok so definitively we should not do this (or exactly like this).

AssetMapper generates during compile-command its config files.

They are not just cache files, as without it in production the assets won't be served.

But it wont be possible to rebuild them with this warmer if some cache:clear is executed (so massive BC break already).

Also these files are not in the cache directory, but in the public/assets directory.. that it self may not be available or writable then.

Some binary may also not be available in production (Sass and/or Typescript users are not a minority), so the recompilation of assets won't be possible there.

@alexandre-daubois
Copy link
Member Author

If I understand correctly, do you advise to not have this cache warmer at all?

@nicolas-grekas
Copy link
Member

After chatting on Slack, what @smnandre means is that in prod mode, the cache warmer will try to compile the assets, which can break, because the preconditions to compile them might not be met (tyically because the assets where compiled ahead of deployment, in a container that doesn't have the required binaries).

This is a concern we should address indeed. Running cache:clear in prod mode shouldn't be destructive as it is now.
For thoughts.

@smnandre
Copy link
Member

Using a cache warmer instead of a manual or explicit “compile” command would be potentially destructive on three levels and useless on one.

Destructive

Deletion of Critical Files
The suggested implementation starts by deleting both public/assets/manifest.json and public/assets/importmap.json. These files are absolutely vital at runtime for generating the import map and managing any asset links.

Risk to Asset Integrity
During compilation, final asset files are written directly to the public/assets directory instead of a dedicated cache or build directory. This means that any problem during the warm-up phase could endanger the integrity of the asset files, potentially resulting in incorrect URL references in the import map or manifest (if they still exist). Additionally, backing up the entire assets directory does not seem like a reasonable solution.

Fallback to Raw Source Assets
If these critical files are still present, the asset mapper will use the raw source assets from the repository. Without compilation, this would break functionality for anyone using SASS, TypeScript, React, or simply needing to bundle files.

Useless Heavy Resource Consumer

Resource-Intensive
The asset-map:compile command, which generates the import map and entry point files, needs to browse and cache metadata about every file in the assets folder. This process is resource-heavy, especially since asset files are often not hosted on the same server/container/image as the PHP code once deployed.

Redundant Caching Efforts
There is no need to populate the cache with data that won’t be used afterward. Symfony applications theoretically have nothing to do with assets once in production. The CompileCommand and the CacheConfigReader were designed to avoid this by ensuring that, in production, the cache is not used and only the necessary files are utilized.


I now strongly believe that, today, a cache warmer (in production) introduces far more problems than it solves.


(P.S. I asked GPT for help to fix my English.. as it seems I have real difficulties expressing my thoughts and being understood these days.)

@nicolas-grekas
Copy link
Member

I don't think the issue is with using a cache warmer on its own. But the implementation of the cache warmer needs to be carefully designed to handle these aspects you raised.

@smnandre
Copy link
Member

I don't think the issue is with using a cache warmer on its own.

Well i'm less and less sure about that to be honest.. As the cache has not much role to play once in production. So there is no real reason to "warm" anything here but there is to ... (not nitpicking) .... "build".

But more generally: of course, it's an AssetMapper / assets "problem" more than a "cache" one...

There are multiple things we should/need to improve on that side:

  • stop mixing env/dev assets
  • allow some form of rollback
  • store internally (var/build ? cache ?) the files I talked about earlier ...
  • ... and having one file per env, removing de facto the problem of switching back and forth from prod to dev (no built prod confg file would be used then)

I also think we need some other changes/improvments (not strictly related to this particular implementation, but somehow addressing the same topic)

  • clarify command names (probably more "component names & reponsabilities"): assets:install deploys bundle assets only, asset-map:compile deploys and compress assets assets:compress, importmap:install does another thing, while composer install has an effect on all this too with flex & stimlus bundle, framework "asset" configuration too, ...
  • add one page par "system" (asset mapper / webpack) named "handle assets with .." , listing very straightforward instructions on how to "install / update / deploy" assets + most frequent use cases / needs
  • introduce extension points filling users need the more (ex: import map entries or splitted stimulus config)

But I think all this requires time and some form of coordination..

Also, i need to say out loud that despite the impression this discussion may give, i'd love to have a real "zero-build" mode available and i'd be the first one to enable it... on my personal projects ;)

@nicolas-grekas
Copy link
Member

Let me close as objections are valid IMHO. We need to figure out another approach...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Turn asset-map:compile into a cache-warmer
4 participants