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.

Sign up for GitHub

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

Update signaling client #18068

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
4c2c1a2
update signaling client
LuChen-Microsoft Oct 6, 2021
359fd39
format code
LuChen-Microsoft Oct 6, 2021
20d7220
fix
LuChen-Microsoft Oct 7, 2021
95e4721
update api view
LuChen-Microsoft Oct 7, 2021
675d5e3
keep the api same
LuChen-Microsoft Oct 7, 2021
0be5c6d
format the code
LuChen-Microsoft Oct 7, 2021
b2e731a
add signaling client options
LuChen-Microsoft Oct 8, 2021
095444e
Merge branch 'Azure:main' into update-signaling-option
LuChen-Microsoft Oct 8, 2021
fbef834
update signaling client
LuChen-Microsoft Oct 6, 2021
d57b78a
format code
LuChen-Microsoft Oct 6, 2021
c294f9f
fix
LuChen-Microsoft Oct 7, 2021
33af64d
update api view
LuChen-Microsoft Oct 7, 2021
35a608e
keep the api same
LuChen-Microsoft Oct 7, 2021
a16d4a6
format the code
LuChen-Microsoft Oct 7, 2021
72d9a54
add signaling client options
LuChen-Microsoft Oct 8, 2021
aff04ed
fix package
LuChen-Microsoft Oct 8, 2021
cf5df84
Merge branch 'update-signaling-option' of https://github.com/LuChen-M…
LuChen-Microsoft Oct 8, 2021
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
View file
Open in desktop

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
View file
Open in desktop
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].
- Addedc `signalingClientOptions` in `ClientOptions`.
LuChen-Microsoft marked this conversation as resolved.
Show resolved Hide resolved

## 1.1.0 (2021-09-15)

### Features Added

Expand Down
2 changes: 1 addition & 1 deletion sdk/communication/communication-chat/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
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
10 changes: 9 additions & 1 deletion sdk/communication/communication-chat/review/communication-chat.api.md
View file
Open in desktop
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
View file
Open in desktop
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
10 changes: 8 additions & 2 deletions sdk/communication/communication-chat/src/models/options.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,15 +17,21 @@ export {
RestListReadReceiptsOptions
};

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 {
signalingClientOptions?: SignalingClientOptions
LuChen-Microsoft marked this conversation as resolved.
Show resolved Hide resolved
}

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

/**
* Options to update a chat thread.
Expand Down
6 changes: 4 additions & 2 deletions sdk/communication/communication-chat/src/signaling/signalingClient.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,14 +4,16 @@
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