NotificationsYou must be signed in to change notification settings
Fork9.6k
Star30.4k

[AssetMapper] avoid caching MappedAsset inside JavaScript Import#52523

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Merged

fabpot merged 1 commit intosymfony:6.4fromweaverryan:asset-mapper-avoid-caching-mapped-asset

Nov 10, 2023

Merged

[AssetMapper] avoid caching MappedAsset inside JavaScript Import#52523

fabpot merged 1 commit intosymfony:6.4fromweaverryan:asset-mapper-avoid-caching-mapped-asset

Nov 10, 2023

Conversation

Copy link

Member

weaverryan commentedNov 9, 2023

Q	A
Branch?	6.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Issues	None
License	MIT

Hi!

Subtle bug. In dev, we cache theMappedAsset objects. Before this PR,MappedAsset->javascriptImports[0]->asset was also aMappedAsset object.

Supposeapp.js importsfoo.js which importsbar.js. Then, the user changesfoo.js to also importbaz.js.

Thefoo.js MappedAsset will now have TWOJavaScriptImport objects (forbar.js andbaz.js). However, when we calculate the preloads, this happens:

We load the cachedMappedAsset forapp.js because this file has not changed
It correctly has 1JavaScriptImport forfoo.js
But theJavaScriptImport->asset property holds an OUT-OF-DATE version of theMappedAsset forfoo.js... because it was cached inside theMappedAsset structure forapp.js. This out-of-dateMappedAsset forfoo.js only contains the ONE JavaScript import.

The result is thatbaz.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 freshMappedAsset from the logical path.

Cheers!

carsonbot added Status: Needs Review AssetMapper Bug labels

Nov 9, 2023

carsonbot added this to the6.4 milestone

Nov 9, 2023

nicolas-grekas reviewed

Nov 9, 2023

View reviewed changes

src/Symfony/Component/AssetMapper/ImportMap/ImportMapGenerator.php OutdatedShow resolvedHide resolved

nicolas-grekas reviewed

Nov 9, 2023

View reviewed changes

src/Symfony/Component/AssetMapper/ImportMap/ImportMapGenerator.php OutdatedShow resolvedHide resolved

nicolas-grekas reviewed

Nov 9, 2023

View reviewed changes

src/Symfony/Component/AssetMapper/ImportMap/JavaScriptImport.php OutdatedShow resolvedHide resolved

weaverryan force-pushed theasset-mapper-avoid-caching-mapped-asset branch fromc6009ce tocaf7fcbCompare

November 10, 2023 02:27

[AssetMapper] Avoid storing & caching MappedAsset objects

bb5ef88

The problem is that these could get out of date, allowing asset Foo to have aJavaScript import for the MappedAsset for Bar, but it is out of date becauseBar has since changed.

weaverryan force-pushed theasset-mapper-avoid-caching-mapped-asset branch fromcaf7fcb tobb5ef88Compare

November 10, 2023 02:31

fabpot merged commita647f55 intosymfony:6.4

Nov 10, 2023

This was referencedNov 10, 2023

Release v6.4.0-BETA3#52538

Merged

Release v7.0.0-BETA3#52541

Merged

weaverryan deleted the asset-mapper-avoid-caching-mapped-asset branch

November 11, 2023 01:35

weaverryan mentioned this pull request

Nov 24, 2023

[AssetMapper] Fix windows ci fail#52711

Merged

xabbuh added a commit that referenced this pull request

Nov 24, 2023

minor#52711[AssetMapper] Fix windows ci fail (weaverryan)

75a6cbb

This PR was merged into the 6.4 branch.Discussion----------[AssetMapper] Fix windows ci fail| Q             | A| ------------- | ---| Branch?       | 6.4| Bug fix?      | yes| New feature?  | no| Deprecations? | no| Issues        | Fixes CI| License       | MITI missed the windows-only test in#52523.Cheers!Commits-------e172b68 [AssetMapper] Fixing out-of-date test on Windows