Skip to content
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

use asset-mapper asset package if available #6665

Closed

Conversation

IndraGunawan
Copy link
Contributor

by default, asset-mapper has all bundles assets https://symfony.com/doc/current/frontend/asset_mapper.html#third-party-bundles-custom-asset-paths

we can skip assets:install command to copy the assets to public dir and reuse what asset-mapper has done

@IndraGunawan IndraGunawan force-pushed the use-mapper-aware-asset-package branch from 0ea0deb to f988f9b Compare December 24, 2024 11:33
@ben29
Copy link

ben29 commented Dec 26, 2024

Nice to have it
Good work :)

@javiereguiluz javiereguiluz added this to the 4.x milestone Dec 26, 2024
@javiereguiluz
Copy link
Collaborator

Thanks. I like this ... but I'm waiting a bit before merging it because I want to think carefully if this could break something for existing apps.

@javiereguiluz
Copy link
Collaborator

I've checked this thoroughly again ... and now I'm going to close this as "won't fix". Let me explain why.

These are the asset links generated before this PR:

And these are the links after this PR:


AssetMapper appends a new hash, but this is not really needed because we already include hashes in our asset links. The root problem of all this is that we don't define AssetMapper as a dependency of this bundle (we'll do that in the future) and that's why we have to do some convoluted process to have the same hashed/versioned assets as in AssetMapper but without usng AssetMapper.


Also, in the Symfony Docs I can read this:

I tested and that limitation isn't fixed after applying the changes of this PR. So, that's why I think this PR doesn't provide in practice a significant improvement over the current situation and that's why I'm closing it.

If this PR solves something that I missed, please tell me and we'll reopen. Thanks!

@IndraGunawan
Copy link
Contributor Author

IndraGunawan commented Jan 4, 2025

hi @javiereguiluz , thanks for the feedback

These are the asset links generated before this PR: ... AssetMapper appends a new hash, but this is not really needed because we already include hashes in our asset links.

I believe this won't be an issue, additional hash from asset-mapper was there by default

The root problem of all this is that we don't define AssetMapper as a dependency of this bundle

asset-mapper is not a hard dependency, https://github.com/EasyCorp/EasyAdminBundle/pull/6665/files#diff-a7bf72e66282b81a3685555cf2fa6fbb80ba61953539e8d6e80b603eb58e0709R399 it will ignore if the asset_mapper is not installed

I tested and that limitation isn't fixed after applying the changes of this PR.

I'm lost here, what limitation are we trying to fix?

because EA uses its own asset package so I used composition here so EA owned asset-package can use asset mapper and not make any issue if asset-mapper is not installed


another thing this PR solved is we can serve the assets from CDN

@javiereguiluz
Copy link
Collaborator

I believe this won't be an issue, additional hash from asset-mapper was there by default

It's not an issue ... but it's not needed either. Our assets already have a hash that depend on their contents.

asset-mapper is not a hard dependency [...] it will ignore if the asset_mapper is not installed

Yes, your PR does this perfectly. I mean that AssetMapper should be a dependency of this bundle. We'll do that in the future.

I'm lost here, what limitation are we trying to fix?

The limitation I mentioned in my comment. This one:

What I meant it: in my opinion, this PR doesn't do anything that we don't have already ... but, if at least solved the limitation mentioned in the docs, we could merge it. But, it doesn't fix it either.


I'm curious about this:

another thing this PR solved is we can serve the assets from CDN

I have Symfony apps with EasyAdmin and they use CDNs. I don't see any problem with the assets. Could you please explain the issues you have with CDNs and current EasyAdmin assets? Thanks!

@IndraGunawan
Copy link
Contributor Author

Could you please explain the issues you have with CDNs and current EasyAdmin assets?

sorry for incomplete statement.

here is the case.

a Symfony apps with EasyAdmin + using FrankenPHP as the app server
in FrankenPHP we can disable the file_server to increase the performance so the web server won't serve the assets file anymore instead we have to upload the assets file to a storage service.

asset-mapper supports this out of the box. run asset-map:compile command, upload the assets file, and set assets.base_url
EasyAdmin is only support PathPackage hence FrankenPHP return 404 while requesting the assets

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

Successfully merging this pull request may close these issues.

3 participants