Skip to content

Added Web Socket inline docs#6

Merged
jackmcintosh merged 4 commits intomasterfrom
dev
Jun 29, 2018
Merged

Added Web Socket inline docs#6
jackmcintosh merged 4 commits intomasterfrom
dev

Conversation

@jackmcintosh
Copy link
Copy Markdown
Contributor

No description provided.

@jackmcintosh jackmcintosh requested review from cjmconie and jjv360 June 27, 2018 10:44
Comment thread src/client/manager/WebSockets.js Outdated
}

/**
* The connection function establishes a connection to the websockets
Copy link
Copy Markdown

@cjmconie cjmconie Jun 28, 2018

Choose a reason for hiding this comment

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

Suggestion: "The connect function establishes a connection to the Web socket."

Comment thread src/client/manager/WebSockets.js Outdated
}

/**
* The handleMessage allows the different types of messages to be returned, stateUpdate,inventory,activity,info
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion:
"The handleMessage function allows the different types of messages to be returned: stateUpdate, inventory, activity, and, info."



/**
* When the connection drops or the Websocket is closed, this function will auto-retry connection until successfully connected
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion:
If the connection drops or the Web socket unexpectedly disconnects, this function will attempt to re-establish the connection using an exponential backoff policy.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Any comments on this one?

Comment thread src/client/manager/WebSockets.js Outdated
}

/**
* Handles the WebSocket close event
Copy link
Copy Markdown

@cjmconie cjmconie Jun 28, 2018

Choose a reason for hiding this comment

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

"WebSocket" > "Web socket"

Comment thread src/client/manager/WebSockets.js Outdated
}

/**
* Forcefully close the WebSocket, It will null the socket and stop the auto connect
Copy link
Copy Markdown

@cjmconie cjmconie Jun 28, 2018

Choose a reason for hiding this comment

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

Forcefully closes the Web socket.

Note: socket will be set to null. Auto connect will be disabled.

* The connection function establishes a connection to the websockets
* @return {Promise<WebSocket>}
*/
connect() {
Copy link
Copy Markdown

@cjmconie cjmconie Jun 28, 2018

Choose a reason for hiding this comment

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

You are correctly guarding against the case where the API consumer calls connect when the Web socket is already connected (using this.socket.readyState !== 3) - preventing multiple connects.

What will happen if the API consumer calls connect (a second time) while the checkToken function is asynchronously fetching a new access token? In that case the socket is not technically 'connected'. It looks to me like the checkToken willl will be called again a new promise will be created?

I had something similar on iOS - let me know if the description is clear enough.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What was the outcome here?

Copy link
Copy Markdown

@cjmconie cjmconie left a comment

Choose a reason for hiding this comment

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

I have added a few comments, mostly around the function descriptions. Let me know they make sense.

@jjv360
Copy link
Copy Markdown
Contributor

jjv360 commented Jun 28, 2018

Maybe the private functions should be marked with @private? The only ones the user should call is close() and connect() ... Also you may need some documentation on the class itself, which shows the user how to register their listeners using .addEventListener("inventory", <callback>) etc...

kyle-mcintosh added 2 commits June 29, 2018 12:32
@jackmcintosh jackmcintosh merged commit d2decba into master Jun 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants