[AssetMapper] Add audit command #51650
Conversation
| ]; | ||
| } | ||
|
|
||
| private function getPackagesVersion(): array |
There was a problem hiding this comment.
This should have a phpdoc describing the return type as array<string, string>
| $subPathRegex = '(?<subpath>/.*)'; | ||
|
|
||
| if ( | ||
| 1 === preg_match("#^https://ga.jspm.io/{$registryRegex}:{$packageRegex}@{$versionRegex}{$subPathRegex}?$#", $url, $matches) |
There was a problem hiding this comment.
why using a registry regex when the only valid value for something supported by the npmjs endpoint would be npm ?
For instance, if you use the gh ecosystem for cdn.jsdelivr.net URLs, you won't get an npm package name after it, so this won't be valid.
There was a problem hiding this comment.
This logic has been moved to resolvers. JsDelivrEsmResolver already get the version when requiring a package. JspmResolver still use a regex (which might be improved I think).
There was a problem hiding this comment.
[Update] Resolver now must implement a getPackageVersion method with extract v
8000
ersion from URL.
src/Symfony/Component/AssetMapper/ImportMap/ImportMapAuditor.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/ImportMapAuditor.php
Outdated
Show resolved
Hide resolved
| $subPathRegex = '(?<subpath>/.*)'; | ||
|
|
||
| if ( | ||
| 1 === preg_match("#^https://ga.jspm.io/{$registryRegex}:{$packageRegex}@{$versionRegex}{$subPathRegex}?$#", $url, $matches) |
There was a problem hiding this comment.
Let's extract this section to the individual PackageResolverInterface classes - e.g. a new getPackageInfo(string $url): ?array which would return... an array with package and version keys or null of a resolver doesn't support the URL (or maybe we make a new small class instead of the array).
We'll need to reuse this logic for an asset-map:outdated command. So putting it in the package resolvers would allow that.
There was a problem hiding this comment.
Given that the interface is an extension point of the package, I suggest using a value object rather than an array shape.
There was a problem hiding this comment.
Maybe we can add this check at "resolvePackages" step, add a ?string $version property to ResolvedImportMapPackage and dump it in importmap.php file. It will look like:
<?php
return [
'@hotwired/stimulus' => [
'url' => 'https://unpkg.com/@hotwired/stimulus@3.2.1/dist/stimulus.js',
'version' => '3.2.1',
],
'file' => [
'path' => 'some/file.js',
],
];WDYT ?
There was a problem hiding this comment.
You can review this in the last commit.
|
This looks like a nice proposal to me. Thanks Hubert. I have a minor comment about the command output. At the bottom we can see this: Two comments:
So, this is how the output would look like: |
We can keep it to inform that, there were n packages that could not be audited.
Good idea. I made changes and updated screenshots in the PR description. |
src/Symfony/Component/AssetMapper/ImportMap/Resolver/JsDelivrEsmResolver.php
Outdated
Show resolved
Hide resolved
| protected function initialize(InputInterface $input, OutputInterface $output): void | ||
| { | ||
| $this->io = new SymfonyStyle($input, $output); | ||
| $this->format = $input->getOption('format'); |
There was a problem hiding this comment.
I'm not sure if setting these types of things on properties in commands is a normal practice. Someone else can please tell me if I'm wrong or right about that :)
There was a problem hiding this comment.
We can move it. IMO it is more readable like this instead of passing $io in every method.
src/Symfony/Component/AssetMapper/ImportMap/ImportMapManager.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/AssetMapper/ImportMap/Resolver/JsDelivrEsmResolver.php
Outdated
Show resolved
Hide resolved
| continue; | ||
| } | ||
| $installed[$entry->importName] ??= []; | ||
| $installed[$entry->importName][] = $entry->version; |
There was a problem hiding this comment.
We can save for another PR, but probably we need to add an optional packageName to ImportMapEntry. 99% of the time the key (importName here) IS the package name. But that's not required. In fact, you can run importmap:require "vue/dist/vue.esm-bundler.js=vue to do this exact thing :).
The idea would be that packageName never appears in importmap.php unless you have changed the key, and then you would need it.
|
We've merged in some changes to AssetMapper - let me know if you need help finishing this / fixing the conflicts :). |
fdc3152 to
c213bbd
Compare
@weaverryan Thanks for the proposal. I rebased this PR, it should be OK now. |
There was a problem hiding this comment.
This is good to go. It's going to conflict with my #51786, which adds a version key to importmap.php (a bit similar to what's done in this PR), but if this is merged first, I can patch things up over in my PR after.
| @@ -57,6 +57,7 @@ public function getEntries(): ImportMapEntries | |||
| isDownloaded: isset($data['downloaded_to']), | |||
| type: $type, | |||
| isEntrypoint: $isEntry, | |||
| version: $data['version'] ?? null, | |||
There was a problem hiding this comment.
version needs to be written also if it's not null in writeEntries().
You can ping me when your PR is merged. I'll update mine. |
ca75ec2 to
6d150fc
Compare
|
Thank you @Jean-Beru. |
…an LE BORGNE) This PR was merged into the 6.4 branch. Discussion ---------- [AssetMapper] Fix unit test in assetmapper auditor | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | yes | New feature? | no | Deprecations? | no | Tickets | | License | MIT Fix a typo in `ImportMapAuditor::audit` introduced in #51650 that causes tests to fail Commits ------- 556486c Fix typo that causes unit test to fail
This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [AssetMapper] Add outdated command | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | | License | MIT As suggested by `@WebMamba`, I added a new command for the AssetMapper component to list outdated 3rd party packages. It is inspired by the `composer outdated` command so I tried to replicate its options as much as I could. It reads the importmap.php and extract packages version to query the https://registry.npmjs.org/%package% API and read the latest version from metadata.  :warning: The code is base on #51650 branch so it is not ready to be merged yet, but I'd be happy to get some reviews and feedback in the meantime. This is my first PR on symfony, so there will probably be a lot to say ! - [ ] gather feedback - [x] wait for #51650 to be merged and rebase - [ ] write documentation Commits ------- 4d32a35 [AssetMapper] Add outdated command
… (weaverryan) This PR was squashed before being merged into the 6.4 branch. Discussion ---------- [AssetMapper] Warn of missing or incompat dependencies | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | None | License | MIT Hi! The aim of the `importmap.php` system is to be a simple way to manage your JS dependencies. But to make it robust enough for production use, it does need a few things - like the `importmap:audit` command in #51650. This PR adds a check, during `importmap:require` and `importmap:update`, that reports any missing dependencies or dependencies with invalid versions. This is necessary so that, if package `A` requires package `B`, their versions don't "drift" over time without you being aware (e.g. you update package `A` to v3 but keep package `B` at v1, even though v3 of `A` requires v2 of `B`). <img width="1266" alt="Screenshot 2023-10-04 at 2 44 04 PM" src="https://github.com/symfony/symfony/assets/121003/3901a070-d092-494a-a7cb-3bfe5d5a99f9"> Built on top of #51786. Cheers! Commits ------- 42dfb9a [AssetMapper] Warn of missing or incompat dependencies
After discussing with @WebMamba, I made this PR to introduce the
importmap:auditcommand inspired fromyarn auditandnpm audit.It reads the
importmap.phpand extract packages version to query thehttps://registry.npmjs.org/-/npm/v1/security/auditsAPI.