-
Notifications
You must be signed in to change notification settings - Fork 198
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
Added optional FeatureSet parameter to eligibility interface #2623
Conversation
uxFeatures: string[]; | ||
/** | ||
* Server Feature set | ||
*/ | ||
serverFeatures: string[]; |
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.
Consider alphabetizing? (This would apply to host sdk changes as well)
@@ -319,6 +338,28 @@ describe('copilot', () => { | |||
|
|||
await expect(promise).rejects.toThrowError('Error deserializing eligibility information'); | |||
}); | |||
|
|||
it('getEligibilityInfo should throw if AppEligibilityInformation.featureSet.serverFeatures or uxFeatures is undefined', async () => { |
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.
Suggest adding two separate unit tests, one for serverFeatures
and one for uxFeatures
being undefined so it's clearer if there was a failure, one which caused it.
…ibrary-js into lakhveer/eligibility
…ibrary-js into lakhveer/eligibility
/** | ||
* UX Feature set | ||
*/ | ||
uxFeatures: string[]; |
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.
Should these be declared as ReadonlyArray<string>
to ensure they aren't accidentally changed and to express the semantic that they're an "input only" property (e.g. they are immutable properties from the host)? #Closed
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.
ReadonlyArray makes sense. Updated the code
/** | ||
* Feature Sets | ||
*/ | ||
featureSet?: FeatureSet; |
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.
What would be the semantic difference between having this property be undefined
vs defined but with two empty array properties?
I'm guessing the answer is something like "this has to have the ?
because older version of the host won't know to send this property so it will show up as undefined then." If that's true, then consider adding documentation for the property that says:
"If this property is undefined, it indicates that the host is an older version that doesn't support this property." #Closed
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.
My understanding is it's the latter, so I think adding a comment is the way to go here.
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.
yep it is for back compatibility. Added the comment!
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.
🕐
…icrosoft-teams-library-js into lakhveer/eligibility
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.
…ibrary-js into lakhveer/eligibility
…icrosoft-teams-library-js into lakhveer/eligibility
For more information about how to contribute to this repo, visit this page.
Description
Added optional parameter featureSet to the AppEligibilityInformation interface.
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: