Added optional FeatureSet parameter to eligibility interface #2623
Conversation
| uxFeatures: string[]; | ||
| /** | ||
| * Server Feature set | ||
| */ | ||
| serverFeatures: string[]; |
There was a problem hiding this comment.
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.
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.
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)?
| /** | ||
| * Feature Sets | ||
| */ | ||
| featureSet?: FeatureSet; |
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
My understanding is it's the latter, so I think adding a comment is the way to go here.
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: