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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: add connection API documentation #300

Closed
wants to merge 1 commit into from
Closed

Conversation

nechaido
Copy link
Member

No description provided.


Don't call this constructor manually unless you use custom tranport.
Recommended approach is to call `connect()` function provided by these modules:
* `net`
Copy link
Member

Choose a reason for hiding this comment

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

I think it's better to put md links here to the function descriptions even though there is no documentation for them right now to avoid missing them later on. See this and this.
Also, maybe you will add stubs for them?

* callback(error, sessionId) — callback function to invoke after the handshake
is completed.
* error: `Error`.
* sessionId: `number`.
Copy link
Member

Choose a reason for hiding this comment

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

Ain't it a string? here

#### connection.inspectInterface(interfaceName, callback)

* interfaceName: `String` — name of an interface to inspect.
* callback(error, proxy) — callback function to invoke after another side responds
Copy link
Member

Choose a reason for hiding this comment

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

...responds to the interface introspection?

* callback(error, proxy) — callback function to invoke after another side responds
interface introspection.
* error: `Error`.
* proxy: `RemoteProxy` — remote proxy to the interface.
Copy link
Member

Choose a reason for hiding this comment

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

remote proxy for the interface?


* interfaceName: `String` — name of an interface.
* eventName: `String` — name of an event.
* args: `Array` — event arguments as an array.
Copy link
Member

Choose a reason for hiding this comment

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

I think 'as an array' is redundant as we already have class written here.


#### connection.stopHeartbeat()

Stop sending heaarbeat messages.
Copy link
Member

Choose a reason for hiding this comment

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

s/heaarbeat/heartbeat/

* event: `string` — name of an event.
* handler(...args).

Used to subscribe on this events:
Copy link
Member

Choose a reason for hiding this comment

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

Used to subscribe to the following events?

* event: `string` — name of an event.
* handler(...args).

Used to subscribe on this events:
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should also add mandatory event arguments?

* event: `string` — name of an event.
* handler(...args).

Used to subscribe on this events:
Copy link
Member

Choose a reason for hiding this comment

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

I think we should use the same style as in Connection here and in other Transport methods as this is just another class description. And in this place specifically, I think it's better to just state that Transport must emit the following events and avoid describing on method (Maybe just state that it has to be an EventEmitter).

* error: `Error`.
* connection: `Connection` — established connection.

Optional, `new SimpleConnectPolicy().connect` will be used if not provided.
Copy link
Member

Choose a reason for hiding this comment

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

Put a link for new SimpleConnectPolicy().connect as in the first comment?

Copy link
Member

@lundibundi lundibundi left a comment

Choose a reason for hiding this comment

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

LGTM

### new Connection(transport, server, client)

* transport: `Transport`.
* server: `Server` — JSTP server instance, used only for server—side parts
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't use em dash in word 'server-side', a hyphen must be used there.

* server: `Server` — JSTP server instance, used only for server—side parts
of connections (optional, but either server or client is required).
* client: [`Client`](./client.md#object-client) — JSTP client instance,
used only for client—side parts of connections (optional,
Copy link
Member

Choose a reason for hiding this comment

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

Same for 'client-side'.


#### getTransport()

* Returns: `Object`.
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this method return Transport?


## Class: SimpleConnectPolicy

Simple generic connection provider. Used for user-side connection.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it'll be better to replace 'user-side' with 'client-side'?

doc/api/tls.md Outdated
`'name@version'` or `{ name, version }`, where version must be
a valid semver range.
* client: [`Client`](./client.md#object-client) — JSTP client instance, used
only for client—side parts of connections (optional, but either server or
Copy link
Member

Choose a reason for hiding this comment

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

Again dash instead of the hyphen in word 'client-side'.

doc/api/net.md Outdated
`'name@version'` or `{ name, version }`, where version must be
a valid semver range.
* client: [`Client`](./client.md#object-client) — JSTP client instance, used
only for client—side parts of connections (optional, but either server or
Copy link
Member

Choose a reason for hiding this comment

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

Dash instead of the hyphen.

doc/api/net.md Outdated
only for client—side parts of connections (optional, but either server or
client is required).
* options: `Object` — will be destructured and passed directly to connFactory.
* callback(error, connection): — Optional callback that will be called when
Copy link
Member

Choose a reason for hiding this comment

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

You have a dash right next to a colon here, looks like the argument type provided in other cases is missing.

@nechaido nechaido changed the title doc: start documenting connection doc: add connection API documentation Sep 26, 2017
* connection: [`Connection`](./connection.md#class-connection).

Should send handshake message with appropriate credentials. You can get client
object provided upon connection creation with connection.client.
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't 'connection.client' be written in a monospaced font?

doc/api/tls.md Outdated
* client: [`Client`](./client.md#object-client) — JSTP client instance, used
only for client-side parts of connections (optional, but either server or
client is required).
* options: `Object` — will be destructured and passed directly to connFactory.
Copy link
Member

Choose a reason for hiding this comment

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

'connFactory' should probably be written in a monospaced font as well.

Copy link
Member

@belochub belochub left a comment

Choose a reason for hiding this comment

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

LGTM

nechaido added a commit that referenced this pull request Sep 26, 2017
PR-URL: #300
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
@nechaido
Copy link
Member Author

Landed in a5b5808.

@nechaido nechaido closed this Sep 26, 2017
@nechaido nechaido deleted the doc-connection branch September 26, 2017 12:01
belochub pushed a commit that referenced this pull request Jan 22, 2018
PR-URL: #300
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
PR-URL: #300
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Mykola Bilochub <nbelochub@gmail.com>
@belochub belochub mentioned this pull request Jan 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants