-
Notifications
You must be signed in to change notification settings - Fork 639
feat!: Add --manifest option for custom manifest to dev server
#3793
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
| }: CheckManifestOptions = {}, | ||
| ): Promise<CheckManifestResult> { | ||
| const manifestPath = pathUtils.join(basePath, NpmSnapFileNames.Manifest); | ||
| const basePath = dirname(manifestPath); |
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.
Is this meant to require that the manifest is in the root of the package still? If it isn't, the following lines break, when looking for the package.json etc
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.
Yes, it is.
| task: async ({ config, spinner }) => { | ||
| await watch(config, { spinner }); | ||
| task: async ({ config, options, spinner }) => { | ||
| const configWithManifest = options.manifestPath |
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.
Should this be merged into the config further up the call stack?
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.
You're right, merging this before calling the watch handler is a lot simpler actually. Not sure why I didn't think of that in the first place 😅
This adds a
--manifestoption to the Snaps CLI dev server (mm-snap watch), which can be used to specify a different manifest file to use for the Snap, e.g., a separate development manifest with initial connections or additional permissions used for testing purposes only.In a follow up PR it will be possible to extend the main manifest file, so it's no longer needed to copy the entire manifest.
Breaking changes
snaps-utils'checkManifestfunction now takes the path to the manifest rather than the base directory containing a manifest. This is needed to allow specifying custom manifests.