-
Notifications
You must be signed in to change notification settings - Fork 641
feat!: Allow manifest to extend another manifest #3802
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
base: main
Are you sure you want to change the base?
Conversation
packages/snaps-utils/src/types.ts
Outdated
| /** | ||
| * A utility type that makes all properties of a type optional, recursively. | ||
| */ | ||
| type DeepPartial<Type> = { |
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.
Do we not have this somewhere already?
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.
Just one that works on objects in snaps-cli's test utils. I couldn't find a general one.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3802 +/- ##
=======================================
Coverage 98.33% 98.34%
=======================================
Files 422 422
Lines 12073 12125 +52
Branches 1875 1884 +9
=======================================
+ Hits 11872 11924 +52
Misses 201 201 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
|
|
||
| return { | ||
| baseManifest, | ||
| extendedManifest: baseManifest, |
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.
Incorrect assignment causes unnecessary manifest self-merge
Medium Severity
When a manifest has no extends field, extendedManifest is incorrectly set to baseManifest instead of undefined. This causes runFixes to call mergeManifests with the manifest against itself (since extendedManifest?.result returns a truthy value). The mergeManifests function's early-return check if (!extendedManifest) then fails, triggering an unnecessary deepmerge operation and creating a new object instead of preserving the original reference.
This is a follow up to #3793, which introduces custom manifests (e.g., one for production, one for development), allowing a custom manifest to extend another manifest. Right now, the entire production manifest would need to be copied to the development manifest. After this change it's possible to do the following:
With a base manifest like:
After which the manifests would be merged like this:
{ "proposedName": "My Snap", "other": "properties", "initialConnections": { "http://example.com": {} } }Right now this is mainly useful for local development, but may be used for bundling production Snaps in the future.
Note
Introduces extendable Snap manifests and updates tooling to load, merge, validate, and watch them.
extendssupport with deep-merge via newloadManifest, returningmergedManifestand involvedfilesmergedManifestand write fixes to the base manifest; adds types (ExtendableManifest,ExtendableSnapFiles)SnapsWatchPluginnow takesmanifestPathand watches files fromloadManifestmergedManifestand computes allowed paths from itsnap.manifest.dev.jsonnowextendsbase manifest; tests/snapshots adjusted accordinglyWritten by Cursor Bugbot for commit 1a67620. This will update automatically on new commits. Configure here.