Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[AssetMapper] Adding debug:assetmap command + normalize paths#50219
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
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fbb5675 toc761332CompareUh oh!
There was an error while loading.Please reload this page.
| $pathRows = []; | ||
| foreach ($this->assetMapperRepository->allDirectories()as$path =>$namespace) { | ||
| $path =$this->relativizePath($path); |
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.
We could makeAssetMapperRepository::getRepositories() able to return the relative paths with some bool argument or even a new method. No big deal though, fine as 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.
That's true - we already have the projectRootDir there. I don't feel too strongly either way :)
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.
if only the debug command needs that, I would keep it in the bug command for now
| $file =rtrim($path,'/').'/'.$localLogicalPath; | ||
| if (file_exists($file)) { | ||
| return$file; | ||
| returnrealpath($file); |
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.
Usingrealpath everywhere now in this file so that all paths use the "real" path. That's just "nice" because you'll get back full paths - instead of potentially getting back file paths like/var/something/else/../other-path/app.js - and is important especially forfindLogicalPath() where we pass in a$filesystemPath and then do some matching by the filesystem path.
weaverryan commentedMay 2, 2023
.. checking on one other edge case bug fix for the repository... |
weaverryan commentedMay 3, 2023
Ok, this is ready - test failures are unrelated. |
fabpot left a comment
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.
Some small suggestions.
src/Symfony/Component/AssetMapper/Command/DebugAssetMapperCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/AssetMapper/Command/DebugAssetMapperCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/AssetMapper/Command/DebugAssetMapperCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/AssetMapper/Command/DebugAssetMapperCommand.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
4807b8b toe990cd1Compareweaverryan commentedMay 5, 2023
This is ready for merge - 2 test failures are unrelated. Thanks for the feedback! |
e990cd1 to7f48565Comparefabpot commentedMay 5, 2023
Thank you@weaverryan. |
… and importmaps (weaverryan)This PR was squashed before being merged into the 6.3 branch.Discussion----------[AssetMapper] Fixing 2 bugs related to the compile command and importmaps| Q | A| ------------- | ---| Branch? | 6.3| Bug fix? | yes| New feature? | no| Deprecations? | none| Tickets | none| License | MIT| Doc PR | not neededFixes 2 bugs:A) Running `assetmap:compile` gives you a `public/assets/importmap.json` for fast importmap dumping on production. But, running `assetmap:compile` again later would re-use the `importmap.json` info to create the new `importmap.json`... meaning it would never update after the first compile.B) The importmap process generates 2 pieces of information: (A) the importmap and (B) which files from the importmap should be preloaded. When we use `assetmap:compile`, we need to dump both pieces of info. So, we now also dump an `importmap.preload.json` file. These are TWO files (and not just one) because we avoid parsing `importmap.json` at runtime: we read the file and dump it straight out. We DO need to parse the "preload" file. So, I've kept them separate.Blocked by#50219, which has some changes that will fix the tests here.Cheers!Commits-------f9f7274 [AssetMapper] Fixing 2 bugs related to the compile command and importmaps
Uh oh!
There was an error while loading.Please reload this page.
For UX, those packages will add their own namespaced paths to the "asset mapper" to make their Stimulus controllers available. So, between your own paths and automatically-added paths, having a way to visualize everything seems helpful.
ALSO
This PR normalizes the paths in asset mapper in 2 ways:
A) Use
/always for logical path - e.g. you would useasset('images/foo.png')everywhere... you wouldn't sayasset('images\\foo.png')on WindowsB) All absolute paths are converted with
realpathinternally. We do some "directory comparisons" (e.g. is/var/fooinside of/var/foo/bar) so having real paths consistently everywhere keeps this working.Tests added for all of these changes
Cheers!