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

Trharris/associated apps #2385

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft
Changes from 2 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
237 changes: 237 additions & 0 deletions packages/teams-js/src/private/associatedApps.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,237 @@
import { sendMessageToParentAsync } from '../internal/communication';
import { ensureInitialized } from '../internal/internalAPIs';
import { ErrorCode, TabInstance } from '../public';
import { runtime } from '../public/runtime';

// Open questions
// 1. I've re-used `TabInstance` from the public API, does that contain all of the information you and app developers might need?
Copy link
Contributor

@ht4963-ms ht4963-ms Jul 2, 2024

Choose a reason for hiding this comment

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

That's great! We should double check the type since it's been a long time since TabInstance was created.

Copy link
Contributor Author

@TrevorJoelHarris TrevorJoelHarris Jul 3, 2024

Choose a reason for hiding this comment

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

Yup, please take a look! It looks like it could work, and since it's what is returned by pages.tabs.getTabInstances there's some nice synergy.

Copy link
Contributor

@debajyoti251088 debajyoti251088 Jul 8, 2024

Choose a reason for hiding this comment

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

Looked at the object thats returned

It does contain what we need

Copy link
Contributor

@richa008 richa008 Jul 25, 2024

Choose a reason for hiding this comment

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

The object that is returned by our resolvers has a lot more fields compared to this. This is missing order, removeUrl, appId, and a few more. I think we would need to create a new object.

// 2. I didn't see any reason to add a `getTabs` function because `pages.tabs.getTabInstances`. Any reason that won't work for you?
TrevorJoelHarris marked this conversation as resolved.
Show resolved Hide resolved
// 3. I've added an `AppTypes[]` param to `addAndConfigureApp` to allow for the host to show different app types to the user. Very open to changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably stick to an enum of sorts. Teams does have the concept of categories when it comes apps. We can look into it and try to map them to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That would be great, it's also fine to not put every possible category into this enum to start with. I thought this might be a nice way to make it easy to expand to future scenarios without requiring a bunch of annoying breaking changes on both ends.

// 4. I've added empty, private `validate` functions for the threadId and TabInstance. Any validation that is possible will help prevent against
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be difficult to do on the TeamsJS layer. On the TMP side we should obviously do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I'm not making it a hard requirement but something to please keep in mind: not having validation in TeamsJS for input as the source of a large amount of bugs and pain as we tried to expand this to other hosts. In many cases, the TeamsJS documentation and validation was missing because it was baked into the Teams codebase, which made it very difficult in practice for other hosts that wanted to support functions to figure out what they needed to do, what valid input was, etc.

Validating it in the TMP layer is better than nothing obviously, but validation in TeamsJS can help us protect against different definitions of "legal input" on other hosts, which can be a real challenge for making cross platform applications.

Like I said: it's possible there's no way to do validation in TeamsJS (although at the very least I'd like you to check for things like empty string), but the more validation you can do in this layer (or in the host sdk) the better!

// bad data being sent to the host. If you have any validation that can be done, please add it there. If you *can* use restrictive types like UUID
// or something, that would be even better.
// 5. I've made the namespace structure an empty `associatedApps` namespace that only contains the `tab` namespace. This was an attempt to leave room for
// expansion in the future for non-tab scenarios that will make it less likely that your callers will need to update their code. Open to opinions though.

// TODO: Add unit tests
// TODO: Add E2E tests

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*
* TODO: Brief description of what this capability does
*/
export namespace associatedApps {
TrevorJoelHarris marked this conversation as resolved.
Show resolved Hide resolved
export enum AppTypes {
meeting = 'meeting',
}

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*
* TODO: Brief description of what this capability does
*/
export namespace tab {
interface ErrorResponse {
TrevorJoelHarris marked this conversation as resolved.
Show resolved Hide resolved
errorCode: ErrorCode;
message?: string; // TODO: Can remove if you don't have a message to send back to the app developer
}

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*
* TODO: Add full description of what this function does, ie "Launches host-owned UI that lets a user select an app, installs it if required,
* runs through app configuration if required, and then associates the app with the threadId provided. If external docs exist, link to them here"
*
* @param threadId Info about where this comes from, links to external docs if available, etc.
* @param appTypes what type of applications to show the user
*
* @returns The TabInstance of the newly associated app
*
* @throws TODO: Description of errors that can be thrown from this function
*/
export function addAndConfigureApp(threadId: string, appTypes: AppTypes[]): Promise<TabInstance> {
TrevorJoelHarris marked this conversation as resolved.
Show resolved Hide resolved
ensureInitialized(runtime); // TODO: add frameContext checks if this is limited to certain contexts such as content

if (!isSupported()) {
throw new Error(ErrorCode.NOT_SUPPORTED_ON_PLATFORM.toString());
}

validateThreadId(threadId);

return sendMessageToParentAsync<[boolean, TabInstance | ErrorResponse]>(
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly
'associatedApps.tab.addAndConfigureApp',
[threadId, appTypes],
).then(([wasSuccessful, response]: [boolean, TabInstance | ErrorResponse]) => {
if (!wasSuccessful) {
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw
const error = response as ErrorResponse;
throw new Error(`Error code: ${error.errorCode}, message: ${error.message ?? 'None'}`);
}
return response as TabInstance;
});
}

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*
* TODO: Add full description of what this function does, ie "Allows the user to go through the tab config process again for the specified app. If
* no config process exists, X happens, etc."
*
* @param tab fill in details
* @param threadId Info about where this comes from, links to external docs if available, etc.
*
* @returns The TabInstance of the newly configured app
*
* @throws TODO: Description of errors that can be thrown from this function
*/
export function reconfigure(tab: TabInstance, threadId: string): Promise<TabInstance> {
ensureInitialized(runtime); // TODO: add frameContext checks if this is limited to certain contexts such as content

if (!isSupported()) {
throw new Error(ErrorCode.NOT_SUPPORTED_ON_PLATFORM.toString());
}

validateTab(tab);
validateThreadId(threadId);

return sendMessageToParentAsync<[boolean, TabInstance | ErrorResponse]>(
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly
'associatedApps.tab.reconfigure',
[tab, threadId],
).then(([wasSuccessful, response]: [boolean, TabInstance | ErrorResponse]) => {
if (!wasSuccessful) {
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw
const error = response as ErrorResponse;
throw new Error(`Error code: ${error.errorCode}, message: ${error.message ?? 'None'}`);
}
return response as TabInstance;
});
}

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*
* TODO: Add full description of what this function does, ie "Renames the tab associated with an app"
*
* @param tab fill in details
* @param threadId Info about where this comes from, links to external docs if available, etc.
*
* @returns The TabInstance of the newly renamed app
TrevorJoelHarris marked this conversation as resolved.
Show resolved Hide resolved
*
* @throws TODO: Description of errors that can be thrown from this function
*/
export function rename(tab: TabInstance, threadId: string): Promise<TabInstance> {
ensureInitialized(runtime); // TODO: add frameContext checks if this is limited to certain contexts such as content

if (!isSupported()) {
throw new Error(ErrorCode.NOT_SUPPORTED_ON_PLATFORM.toString());
}

validateTab(tab);
validateThreadId(threadId);

return sendMessageToParentAsync<[boolean, TabInstance | ErrorResponse]>(
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly
'associatedApps.tab.rename',
[tab, threadId],
).then(([wasSuccessful, response]: [boolean, TabInstance | ErrorResponse]) => {
if (!wasSuccessful) {
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw
const error = response as ErrorResponse;
throw new Error(`Error code: ${error.errorCode}, message: ${error.message ?? 'None'}`);
}
return response as TabInstance;
});
}

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*
* TODO: Add full description of what this function does, ie "Removes a tab associated with an app, must be called on an app tab, etc."
*
* @param tab fill in details
* @param threadId Info about where this comes from, links to external docs if available, etc.
*
* @throws TODO: Description of errors that can be thrown from this function
*/
export function remove(tab: TabInstance, threadId: string): Promise<void> {
ensureInitialized(runtime); // TODO: add frameContext checks if this is limited to certain contexts such as content

if (!isSupported()) {
throw new Error(ErrorCode.NOT_SUPPORTED_ON_PLATFORM.toString());
}

validateTab(tab);
validateThreadId(threadId);

return sendMessageToParentAsync<[boolean, TabInstance | ErrorResponse]>(
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly
'associatedApps.tab.remove',
[tab, threadId],
).then(([wasSuccessful, response]: [boolean, TabInstance | ErrorResponse]) => {
TrevorJoelHarris marked this conversation as resolved.
Show resolved Hide resolved
if (!wasSuccessful) {
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw
const error = response as ErrorResponse;
throw new Error(`Error code: ${error.errorCode}, message: ${error.message ?? 'None'}`);
}
});
}

/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*/
export function isSupported(): boolean {
throw new Error('Not implemented');
}

function validateThreadId(threadId: string) {
// TODO: Any checks you can do on threadId to guarantee valid (not null, not empty, not undefined, format if possible, etc.)
/*
if (threadId is not valid) {
throw new Error(`${threadId} is not a valid threadId`);
}
*/
}

function validateTab(tabInstance: TabInstance) {
// TODO: Any checks you can do on TabInstance to guarantee valid (not null, not empty, not undefined, all required properties set to legal values, etc.)
/*
if (tabInstance is not valid) {
throw new Error(`TabInstance ${tabInstance.internalTabInstanceId} is not a valid, extra detail if available`);
}
*/
}
}
/**
* @hidden
* @internal
* @beta
* Limited to Microsoft-internal use
*/
export function isSupported(): boolean {
throw new Error('Not implemented');
}
}
Loading