-
Notifications
You must be signed in to change notification settings - Fork 199
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
Trharris/associated apps #2385
Changes from 5 commits
a16e34e
a6102ca
0ee2470
deec7f7
0e274ab
1a3e9e8
08a6610
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, SdkError, 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? | ||
// 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. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 { | ||
// 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 | SdkError]>( | ||
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly | ||
'associatedApps.tab.addAndConfigureApp', | ||
[threadId, appTypes], | ||
).then(([wasSuccessful, response]: [boolean, TabInstance | SdkError]) => { | ||
if (!wasSuccessful) { | ||
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw | ||
const error = response as SdkError; | ||
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 | SdkError]>( | ||
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly | ||
'associatedApps.tab.reconfigure', | ||
[tab, threadId], | ||
).then(([wasSuccessful, response]: [boolean, TabInstance | SdkError]) => { | ||
if (!wasSuccessful) { | ||
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw | ||
const error = response as SdkError; | ||
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 tab | ||
* | ||
* @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 | SdkError]>( | ||
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly | ||
'associatedApps.tab.rename', | ||
[tab, threadId], | ||
).then(([wasSuccessful, response]: [boolean, TabInstance | SdkError]) => { | ||
if (!wasSuccessful) { | ||
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw | ||
const error = response as SdkError; | ||
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 | SdkError]>( | ||
'apiVersionTag', // TODO: see uses of getApiVersionTag in other files to do this correctly | ||
'associatedApps.tab.remove', | ||
[tab, threadId], | ||
).then(([wasSuccessful, response]: [boolean, SdkError]) => { | ||
if (!wasSuccessful) { | ||
// TODO: Can handle error codes differently here, for example if you don't want "user cancelled" to throw | ||
const error = response as SdkError; | ||
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'); | ||
} | ||
} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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
microsoft-teams-library-js/packages/teams-js/src/public/interfaces.ts
Line 17 in 931a483
There was a problem hiding this comment.
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.