-
Notifications
You must be signed in to change notification settings - Fork 197
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
base: main
Are you sure you want to change the base?
Trharris/associated apps #2385
Conversation
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? |
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
export interface TabInstance { |
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.
// 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? | ||
// 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 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.
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 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.
// 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? | ||
// 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. | ||
// 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 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.
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, 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!
* | ||
* @throws TODO: Description of errors that can be thrown from this function | ||
*/ | ||
export function addAndConfigure(hostEntityId: string, appTypes: AppTypes[]): Promise<TabInstance> { |
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.
Why call it hostEntityId and not threadId here?
|
||
// Open questions | ||
// 1. According to Debo, `TabInstance` from the public API looks like it would work. Helen asked Debo to follow up about getting more recent fields added. | ||
// 2. I didn't see any reason to add a `getTabs` function because `pages.tabs.getTabInstances`. Any reason that won't work for you? |
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.
getTabInstances may not work for us, I don't think it has been updated to return configurable+static tabs.
For more information about how to contribute to this repo, visit this page.
Description
Proposal for adding a new capability for 1P apps to add associated app tabs to themselves.
Main changes in the PR:
Validation
Validation performed:
Unit Tests added:
<Yes/No>
End-to-end tests added:
<Yes/No>
Additional Requirements
Change file added:
<Yes/No>
Related PRs:
Next/remaining steps:
Screenshots: