Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Promises to RTMClient start and disconnect #611

Merged
merged 3 commits into from Aug 24, 2018
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
56 changes: 42 additions & 14 deletions src/RTMClient.ts
Expand Up @@ -131,10 +131,7 @@ export class RTMClient extends EventEmitter {

return this.autoReconnect && isRecoverable;
})
.ignore().withAction(() => {
// dispatch 'failure' on parent machine to transition out of this submachine's states
this.stateMachine.handle('failure');
})
.transitionTo('failed')
.state('authenticated')
.onEnter((_state, context) => {
this.authenticated = true;
Expand All @@ -145,6 +142,11 @@ export class RTMClient extends EventEmitter {
})
.on('websocket open').transitionTo('handshaking')
.state('handshaking') // a state in which to wait until the 'server hello' event
.state('failed')
.onEnter((_state, context) => {
// dispatch 'failure' on parent machine to transition out of this submachine's states
this.stateMachine.handle('failure', context.error);
})
.global()
.onStateEnter((state) => {
this.logger.debug(`transitioning to state: connecting:${state}`);
Expand Down Expand Up @@ -231,10 +233,15 @@ export class RTMClient extends EventEmitter {
.onSuccess().transitionTo('connecting')
.onExit(() => this.teardownWebsocket())
.global()
.onStateEnter((state) => {
.onStateEnter((state, context) => {
this.logger.debug(`transitioning to state: ${state}`);
// Emits events: `disconnected`, `connecting`, `connected`, 'disconnecting', 'reconnecting'
this.emit(state);
if (state === 'disconnected') {
// Emits a `disconnected` event with a possible error object (might be undefined)
this.emit(state, context.eventPayload);
} else {
// Emits events: `connecting`, `connected`, `disconnecting`, `reconnecting`
this.emit(state);
}
})
.getConfig();

Expand Down Expand Up @@ -354,8 +361,7 @@ export class RTMClient extends EventEmitter {
* Begin an RTM session using the provided options. This method must be called before any messages can
* be sent or received.
*/
public start(options?: methods.RTMStartArguments | methods.RTMConnectArguments): void {
// TODO: should this return a Promise<WebAPICallResult>?
public start(options?: methods.RTMStartArguments | methods.RTMConnectArguments): Promise<WebAPICallResult> {
// TODO: make a named interface for the type of `options`. it should end in -Options instead of Arguments.

this.logger.debug('start()');
Expand All @@ -365,18 +371,40 @@ export class RTMClient extends EventEmitter {

// delegate behavior to state machine
this.stateMachine.handle('start');

// return a promise that resolves with the connection information
return new Promise((resolve, reject) => {
this.once('authenticated', (result) => {
this.removeListener('disconnected', reject);
resolve(result);
});
this.once('disconnected', (err) => {
this.removeListener('authenticated', resolve);
reject(err);
});
});
}

/**
* End an RTM session. After this method is called no messages will be sent or received unless you call
* start() again later.
*/
public disconnect(): void {
// TODO: should this return a Promise<void>?
this.logger.debug('manual disconnect');
public disconnect(): Promise<void> {
return new Promise((resolve, reject) => {
this.logger.debug('manual disconnect');

// resolve (or reject) on disconnect
this.once('disconnected', (err) => {
if (err instanceof Error) {
reject(err);
} else {
resolve();
}
});

// delegate behavior to state machine
this.stateMachine.handle('explicit disconnect');
// delegate behavior to state machine
this.stateMachine.handle('explicit disconnect');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

any reason to kick this event off inside the returned Promise's constructor, as opposed to the line before the return statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the web socket immediately errors on closing (that is, on the same tick of the event loop as handle is called) then the event handler won't be registered by the time the state machine transitions, and the promise will never reject.

Alternatively, the web socket could also immediately close (e.g. when mocked for testing) and the same problem would happen, though the promise wouldn't resolve.

I'm just being cautious that the reactive bit of code is set up before I set off the chain reaction. Don't want any race conditions happening 馃槄

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憣sounds reasonable to me!

});
}

/**
Expand Down