-
-
Couldn't load subscription status.
- Fork 388
[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
packagefield containing name and version to separatenameandversionfields 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.
| 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)), |
Copilot
AI
Sep 9, 2025
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().
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; |
Copilot
AI
Sep 9, 2025
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.
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
| 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'] ?? [], |
Copilot
AI
Sep 9, 2025
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.
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.jsonfiles 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.jsonfiles to use anameand optionalversionfield for PHP dependencies instead of a singlepackagefield. 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.jsonschema to:nameinstead ofpackagefor PHP dependencies.Validation Improvements:
Assert::npmPackageNamemethod to validate NPM package names, ensuring only valid names are accepted when defining NPM dependencies.