-
-
Notifications
You must be signed in to change notification settings - Fork 384
[Toolkit] Add npm & importmap package dependencies #3071
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
[Toolkit] Add npm & importmap package dependencies #3071
Conversation
33b6f00
to
9d8d870
Compare
9d8d870
to
f783a4f
Compare
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.
Pull Request Overview
This PR introduces support for NPM and importmap package dependencies in the Symfony UX Toolkit. The main purpose is to standardize dependency definitions across manifest files and enable frontend package management alongside existing PHP package dependencies.
- Standardizes dependency format from a
package
field containing name and version to separatename
andversion
fields for PHP dependencies - Adds new dependency types for NPM and importmap packages with appropriate validation
- Updates installation and documentation generation to handle all three dependency types
Reviewed Changes
Copilot reviewed 42 out of 42 changed files in this pull request and generated 3 comments.
Show a summary per file
File | Description |
---|---|
Multiple shadcn manifest.json files | Updated PHP dependency format to use separate name and version fields |
schema-kit-recipe-v1.json | Added schema definitions for NPM and importmap dependency types |
src/Toolkit/src/Dependency/ | Added new dependency classes for NPM and importmap packages |
src/Toolkit/src/Assert.php | Added NPM package name validation |
src/Toolkit/src/Recipe/RecipeManifest.php | Updated parsing logic to handle all dependency types |
Pool/PoolResolver classes | Extended to manage NPM and importmap dependencies |
Command classes | Enhanced output to display all dependency types |
Test files | Added comprehensive test coverage for new functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -59,7 +59,7 @@ public function getContent(): string | |||
$this->component->manifest->description, | |||
current($examples), | |||
$this->toolkitService->renderInstallationSteps($this->kitId, $this->component), | |||
dump(preg_replace('/^```twig.*\n/', '```twig'.\PHP_EOL, current($examples))), | |||
preg_replace('/^```twig.*\n/', '```twig'.\PHP_EOL, current($examples)), |
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.
Removing the dump()
call is correct, but this code will fail if $examples
is empty since current()
will return false
. Consider adding a null coalesce operator or checking if $examples
is not empty before calling current()
.
Copilot uses AI. Check for mistakes.
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.
Yeah but we assume we will always have one example
if (isset($this->phpPackageDependencies[$dependency->name]) && !$dependency->isHigherThan($this->phpPackageDependencies[$dependency->name])) { | ||
return; |
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.
The logic has been inverted but this creates a bug. Previously, packages were updated when the new version was higher. Now they're only updated when the new version is NOT higher, which means lower versions will overwrite higher versions. The condition should be $dependency->isHigherThan($this->phpPackageDependencies[$dependency->name])
without the negation.
Copilot uses AI. Check for mistakes.
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.
No it's correct
return new self( | ||
type: $type, | ||
name: $data['name'] ?? throw new \InvalidArgumentException('Property "name" is required.'), | ||
description: $data['description'] ?? throw new \InvalidArgumentException('Property "description" is required.'), | ||
copyFiles: $data['copy-files'] ?? throw new \InvalidArgumentException('Property "copy-files" is required.'), | ||
copyFiles: $data['copy-files'] ?? [], |
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.
Making copy-files
optional by defaulting to an empty array is a breaking change. The previous behavior required this field, and tests indicate this was intentional. Consider reverting this change to maintain backward compatibility, or update the schema and documentation if this change is intentional.
Copilot uses AI. Check for mistakes.
I needed to reference a npm dependency while writing on new recipes for the Shadcn kit:
This pull request standardizes the way dependencies are defined across multiple
manifest.json
files for shadcn UI kits and updates the schema to support more flexible dependency definitions. It also introduces validation for NPM package names in the codebase.Manifest and Schema Standardization:
manifest.json
files to use aname
and optionalversion
field for PHP dependencies instead of a singlepackage
field. This change improves consistency and clarity in dependency definitions. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18]schema-kit-recipe-v1.json
schema to:name
instead ofpackage
for PHP dependencies.Validation Improvements:
Assert::npmPackageName
method to validate NPM package names, ensuring only valid names are accepted when defining NPM dependencies.