173 messenger refactor #245
Conversation
|
@sqweelygig, status checks have failed for this PR. Please make appropriate changes and recommit. |
|
@sqweelygig Sorry, I just have not had time to get round to this this week, been an utterly manic week. I'll be doing this first thing Monday though (obviously won't be a full review, just architecture). |
package-lock.json
Outdated
| @@ -0,0 +1,5893 @@ | |||
| { | |||
There was a problem hiding this comment.
NPM 5's package-lock.json is currently (unless I've missed something) a bit 'up in the air' (npm/npm#16866). I'd rather not commit the lock file until it's extremely clear as to what the flow is (there doesn't appear to be consistency).
Additionally, we may only want to eventually commit to the application version of ProcBots, and not the library. It needs discussion.
There was a problem hiding this comment.
That one slipped through, not deliberate, thanks for catching. For now I have put it in the .gitignore.
There was a problem hiding this comment.
Note to self: done.
| @@ -0,0 +1,2 @@ | |||
| import TypedError = require('typed-error'); | |||
There was a problem hiding this comment.
I'm against this on the principal that it's a very specific error class, and we'd end up with them dotted all over the place. I'm far more in favour of including any error type that's associated with a class as a definition in the class itself (look at https://github.com/resin-io-modules/resin-procbots/blob/master/lib/services/github.ts#L60 as an example).
For that reason, I think there should be a ProcBotError type, that exists in procbot.ts. This would include both code and message, where code is from a const enumerated type defined in procbot-types.ds. This is therefore one such ProcBotError error type.
There was a problem hiding this comment.
Note to self: Done.
lib/services/service-utilities.ts
Outdated
| const context = data.contexts[this.serviceName] as ServiceEmitContext; | ||
| if (!context) { | ||
| return Promise.resolve({ | ||
| err: new ContextAbsentError(`No ${this.serviceName} context`), |
There was a problem hiding this comment.
As previous comment, this could by a specific ProcBotError, eg. new ProcBotError({ code: 123, message: No ${this.serviceName} context)}).
There was a problem hiding this comment.
Note to self: Done.
lib/services/service-utilities.ts
Outdated
| * - provide a `apiHandle` getter, which should return the underlying object that executes the requests. | ||
| * - enqueue your events with `context` and `event`, as per `UtilityServiceEvent`. | ||
| */ | ||
| export abstract class ServiceUtilities<T> extends WorkerClient<T> implements ServiceListener, ServiceEmitter { |
There was a problem hiding this comment.
@pimterry pointed out that this isn't really utilities in the strict sense of the word, as Services inherit from it if they want to use it. I don't really like AbstractService as a Service doesn't have to inherit from it. ServiceParent? ServiceFramework? I have no idea! 😺
There was a problem hiding this comment.
Note to self: Done, ServiceScaffold.
| import { ServiceEvent } from './service-types'; | ||
|
|
||
| /** Data object the ensures the existence of context and event for serviceUtilities. */ | ||
| export interface UtilityServiceEvent extends ServiceEvent { |
There was a problem hiding this comment.
The event should probably use the same nomenclature as the class that uses it, ServiceUtilityEvent, maybe.
There was a problem hiding this comment.
Note to self: Done
lib/services/discourse.ts
Outdated
| } | ||
| public request(requestOptions: UrlOptions & RequestPromiseOptions): Promise<DiscourseResponse> { | ||
| // This type massages request-promise into bluebird | ||
| return new Promise((resolve) => { |
There was a problem hiding this comment.
If we're using request-promise, why is there a new Promise here instead of just returning the result of request?
There was a problem hiding this comment.
Note to self: Done
lib/services/flowdock.ts
Outdated
| public constructor(data: FlowdockConstructor, listen = true ) { | ||
| super(listen); | ||
| this.data = data; | ||
| constructor(data: FlowdockConnectionDetails, listen: boolean) { |
There was a problem hiding this comment.
As per Front comments, listen could be an option in FlowdockConnectionDetails.
lib/services/flowdock.ts
Outdated
| this.session = new Session(data.token); | ||
| this.org = data.organization; | ||
| if (listen) { | ||
| this.startListening(); |
There was a problem hiding this comment.
Drop startListening() method and add the code into this block.
There was a problem hiding this comment.
Note to self: Done
lib/services/front.ts
Outdated
| export function createMessageService(data: FrontConstructor): Messenger { | ||
| return new FrontService(data, false); | ||
| } | ||
| // /* |
There was a problem hiding this comment.
I've got a slightly modified (maybe) version of this in the NotifyBot PR: https://ghttps://github.com/resin-io-modules/resin-procbots/pull/243ithub.com/resin-io-modules/resin-procbots/pull/243
There was a problem hiding this comment.
The Front service has currently been put temporarily out of scope, as indicated by the massive commenting, and will be coming back into the fold in a couple of days.
lib/services/messenger.ts
Outdated
| this.translators[serviceName] = Translator.createTranslator(serviceName, subConnectionDetails, data.dataHubs); | ||
| }); | ||
| if (listen) { | ||
| this.startListening(data.subServices); |
There was a problem hiding this comment.
Move the startListening() method into this body.
There was a problem hiding this comment.
Note to self: Done
|
@sqweelygig, status checks have failed for this PR. Please make appropriate changes and recommit. |
|
@sqweelygig, status checks have failed for this PR. Please make appropriate changes and recommit. |
|
@sqweelygig, status checks have failed for this PR. Please make appropriate changes and recommit. |
THIS PR SHOULD NOT BE MERGED
I'm creating this PR to assist in providing structured feedback on the current state of SyncBot.
At the moment it creates new threads between flowdock and forums. To add to this is front, new comments, backporting and rebasing (not necessarily in that order)