fix: allow extension to register multiple publish targets #4424
Conversation
a-b-r-o-w-n
requested review from
beyackle,
boydc2014,
cwhitten,
GeoffCoxMSFT,
hatpick,
srinaath and
tonyanziano
as code owners
| registration.addPublishMethod(plugin1); | ||
| registration.addPublishMethod(plugin2); |
| @@ -25,6 +24,7 @@ | |||
| }, | |||
| "dependencies": { | |||
| "@botframework-composer/types": "*", | |||
| "@types/passport": "^1.0.3", | |||
There was a problem hiding this comment.
This actually needs to be in the dependencies because we reference it in an exported type.
| log('registering publish method', this.name); | ||
| this.context.extensions.publish[plugin.customName || this.name] = { | ||
| if (this.context.extensions.publish[plugin.name]) { | ||
| throw new Error(`Duplicate publish method. Cannot register publish method with name ${plugin.name}.`); |
There was a problem hiding this comment.
nit: if this is surfaced to the UI, maybe use formatMessage
There was a problem hiding this comment.
Couple of things:
- It doesn't get to the UI. It will end up in the terminal if this is thrown.
- Our server is not yet set up to handle i18n.
There was a problem hiding this comment.
As far as I've seen, messages that get thrown out to the console (like those inside exceptions) don't need to be localized. Only strings that get displayed to a normal user do.
| @@ -6,8 +6,8 @@ import path from 'path'; | |||
| import { v4 as uuid } from 'uuid'; | |||
There was a problem hiding this comment.
I highly recommend avoid using arrow functions for class methods.
There was a problem hiding this comment.
Noted. I just did minimal updates to this file, so feel free to open a PR with that change.
| class LocalPublisher { | ||
| class LocalPublisher implements PublishPlugin<PublishConfig> { | ||
| public name = 'localPublisher'; | ||
| public description = 'Publish bot to local runtime'; | ||
| static runningBots: { [key: string]: RunningBot } = {}; |
There was a problem hiding this comment.
nit: Record<string, RunningBot>
There was a problem hiding this comment.
I always forget about Record. However, since this PR is scoped to the publishing name and description, I am going to leave as is.
…4424) * build extension bundle when building client * enable re-use of plugin host * require name and description for publish plugins * update publish plugins * add multiple publish plugins in sample-ui-plugin * update azure publish description * wrap file path in quote for windows compat * fix tests
Description
Requires name and description for publish targets.
Task Item
fixes #4394
Screenshots