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

Update signaling client #18068

Merged
merged 17 commits into from
Oct 8, 2021
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions common/config/rush/pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 7 additions & 1 deletion sdk/communication/communication-chat/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
# Release History

## 1.1.0 ((2021-09-15)
## 1.1.1 (Not released)

### Features Added
- Updated to @azure/[email protected].
- Added `signalingClientOptions` in `ClientOptions`.

## 1.1.0 (2021-09-15)

### Features Added

Expand Down
4 changes: 2 additions & 2 deletions sdk/communication/communication-chat/package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@azure/communication-chat",
"version": "1.1.0",
"version": "1.1.1",
"description": "Azure client library for Azure Communication Chat services",
"sdk-type": "client",
"main": "dist/index.js",
Expand Down Expand Up @@ -65,7 +65,7 @@
"dependencies": {
"@azure/abort-controller": "^1.0.0",
"@azure/communication-common": "^1.1.0",
"@azure/communication-signaling": "1.0.0-beta.10",
"@azure/communication-signaling": "1.0.0-beta.11",
"@azure/core-auth": "^1.3.0",
"@azure/core-client": "^1.0.0",
"@azure/core-rest-pipeline": "^1.1.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ export class ChatClient {

// @public
export interface ChatClientOptions extends CommonClientOptions {
// (undocumented)
signalingClientOptions?: SignalingClientOptions;
}

// @public
Expand Down Expand Up @@ -141,7 +143,7 @@ export class ChatThreadClient {
}

// @public
export interface ChatThreadClientOptions extends ChatClientOptions {
export interface ChatThreadClientOptions extends CommonClientOptions {
}

// @public
Expand Down Expand Up @@ -264,6 +266,12 @@ export interface SendTypingNotificationOptions extends OperationOptions {
senderDisplayName?: string;
}

// @public (undocumented)
export interface SignalingClientOptions {
// (undocumented)
environment?: string;
}

// @public
export interface UpdateMessageOptions extends OperationOptions {
content?: string;
Expand Down
6 changes: 3 additions & 3 deletions sdk/communication/communication-chat/src/chatClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ export class ChatClient {
const authPolicy = createCommunicationTokenCredentialPolicy(this.tokenCredential);
this.client.pipeline.addPolicy(authPolicy);

this.signalingClient = getSignalingClient(credential, logger);
this.signalingClient = getSignalingClient(credential, logger, options.signalingClientOptions);
}

/**
Expand Down Expand Up @@ -238,7 +238,7 @@ export class ChatClient {
}

this.isRealtimeNotificationsStarted = true;
this.signalingClient.start();
await this.signalingClient.start();
LuChen-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
this.subscribeToSignalingEvents();
}

Expand All @@ -252,7 +252,7 @@ export class ChatClient {
}

this.isRealtimeNotificationsStarted = false;
this.signalingClient.stop();
await this.signalingClient.stop();
this.emitter.removeAllListeners();
}

Expand Down
16 changes: 14 additions & 2 deletions sdk/communication/communication-chat/src/models/options.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,27 @@ export {
RestListReadReceiptsOptions
};

/**
* Options for signaling client.
*/
export interface SignalingClientOptions {
environment?: string;
Copy link
Member

Choose a reason for hiding this comment

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

Could you please document it

Copy link
Member Author

Choose a reason for hiding this comment

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

We actually do not want normal customer to use it. It's mainly for internal test. @juancamilor

Copy link
Member

Choose a reason for hiding this comment

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

If that is the case, I would highly suggest to not put it in the type.

Also, could you please explain more why you need it for tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's for dynamics team to have their apps pointing to the int service url.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, did you consider removing the option from the type? The test team can always pass it in a type-unsafe way by casting to any.

Copy link
Member Author

Choose a reason for hiding this comment

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

I am thinking we may want to add other options for signaling client later. And these options are only for signaling client creation. If it's unsafe this way, I can put this directly into ClientOptions.

}

/**
* Options to create chat client.
*/
export interface ChatClientOptions extends CommonClientOptions {}
export interface ChatClientOptions extends CommonClientOptions {
/**
* Options for the signaling client.
*/
signalingClientOptions?: SignalingClientOptions;
}

/**
* Options to create chat thread client.
*/
export interface ChatThreadClientOptions extends ChatClientOptions {}
export interface ChatThreadClientOptions extends CommonClientOptions {}

/**
* Options to update a chat thread.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,18 @@
import { CommunicationSignalingClient, SignalingClient } from "@azure/communication-signaling";
import { CommunicationTokenCredential } from "@azure/communication-common";
import { AzureLogger } from "@azure/logger";
import { SignalingClientOptions } from "../models/options";

export const getSignalingClient = (
credential: CommunicationTokenCredential,
logger: AzureLogger
logger: AzureLogger,
options?: SignalingClientOptions
): SignalingClient | undefined => {
if (typeof navigator !== "undefined" && navigator.product === "ReactNative") {
// In React Native
return new CommunicationSignalingClient(credential, logger);
return new CommunicationSignalingClient(credential, logger, {
environment: options?.environment ?? undefined
});
}

// In node js
Expand Down