Skip to content

[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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
May 5, 2023

Conversation

weaverryan
Copy link
Member

@weaverryan weaverryan commented May 2, 2023

Q A
Branch? 6.3
Bug fix? no
New feature? yes
Deprecations? no
Tickets None
License MIT
Doc PR Docs are TODO already for the component

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.

Screenshot 2023-05-02 at 4 15 49 PM

ALSO

This PR normalizes the paths in asset mapper in 2 ways:

A) Use / always for logical path - e.g. you would use asset('images/foo.png') everywhere... you wouldn't say asset('images\\foo.png') on Windows
B) All absolute paths are converted with realpath internally. We do some "directory comparisons" (e.g. is /var/foo inside of /var/foo/bar) so having real paths consistently everywhere keeps this working.

Tests added for all of these changes

Cheers!

@carsonbot carsonbot added this to the 6.3 milestone May 2, 2023
@weaverryan weaverryan force-pushed the asset-mapper-debug-command branch from fbb5675 to c761332 Compare May 2, 2023 20:19

$pathRows = [];
foreach ($this->assetMapperRepository->allDirectories() as $path => $namespace) {
$path = $this->relativizePath($path);
Copy link
Member

Choose a reason for hiding this comment

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

We could make AssetMapperRepository::getRepositories() able to return the relative paths with some bool argument or even a new method. No big deal though, fine as is

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's true - we already have the projectRootDir there. I don't feel too strongly either way :)

Copy link
Member

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

@@ -55,7 +55,7 @@ public function find(string $logicalPath): ?string

$file = rtrim($path, '/').'/'.$localLogicalPath;
if (file_exists($file)) {
return $file;
return realpath($file);
Copy link
Member Author

Choose a reason for hiding this comment

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

Using realpath 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 for findLogicalPath() where we pass in a $filesystemPath and then do some matching by the filesystem path.

@weaverryan
Copy link
Member Author

.. checking on one other edge case bug fix for the repository...

@weaverryan weaverryan changed the title [AssetMapper] Adding debug:assetmap command [AssetMapper] Adding debug:assetmap command + normalize paths May 3, 2023
@weaverryan
Copy link
Member Author

Ok, this is ready - test failures are unrelated.

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

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

Some small suggestions.

@weaverryan weaverryan force-pushed the asset-mapper-debug-command branch from 4807b8b to e990cd1 Compare May 5, 2023 09:59
@weaverryan
Copy link
Member Author

This is ready for merge - 2 test failures are unrelated. Thanks for the feedback!

@fabpot fabpot force-pushed the asset-mapper-debug-command branch from e990cd1 to 7f48565 Compare May 5, 2023 10:34
@fabpot
Copy link
Member

fabpot commented May 5, 2023

Thank you @weaverryan.

@fabpot fabpot merged commit 4cc4973 into symfony:6.3 May 5, 2023
@weaverryan weaverryan deleted the asset-mapper-debug-command branch May 5, 2023 16:45
nicolas-grekas added a commit that referenced this pull request May 5, 2023
… 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 needed

Fixes 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
@fabpot fabpot mentioned this pull request May 7, 2023
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.

5 participants