Support for injecting WebTokenRequest.Properties

CommunityToolkit / Graph-Controls

Set of Helpers and Controls for Windows development using the Microsoft Graph.

https://docs.microsoft.com/en-us/windows/communitytoolkit/graph/overview

Other

155 stars 39 forks source link

Support for injecting WebTokenRequest.Properties #207

Closed Richasy closed 1 year ago

Richasy commented 1 year ago

Fixes #

Support users to inject properties when building WebTokenRequest internally. In addition, sometimes the properties applicable to MSA do not support AAD, so the two are separated for injection separately.

PR Type

What kind of change does this PR introduce?

Feature

What is the current behavior?

Users cannot intervene in the construction process of WebTokenRequest.

What is the new behavior?

Support adding custom properties when constructing WebTokenRequest.

PR Checklist

Please check if your PR fulfills the following requirements:

[ ] Tested code with current supported SDKs
[ ] Sample in sample app has been added / updated (for bug fixes / features)
- [ ] Icon has been created (if new sample) following the Thumbnail Style Guide and templates
[ ] Tests for the changes have been added (for bug fixes / features) (if applicable)
[ ] Header has been added to all new source files (run build/UpdateHeaders.bat)
[x] Contains NO breaking changes

Other information

ghost commented 1 year ago

Thanks Richasy for opening a Pull Request! The reviewers will test the PR and highlight if there is any merge conflict or changes required. If the PR is approved we will proceed to merge the pull request 🙌

michael-hawker commented 1 year ago

Hey @Richasy just noticed your main is fairly out-of-date. Rebased this PR here, but be sure to stay in sync.

@Sergio0694 any thoughts on the initialization vs. null for better nullability practices here?

Sergio0694 commented 1 year ago

The current setup seems fine, given the type already exists and it's public, and it's already mutable. Making those two empty dictionaries makes sense to avoid callers having to check for null, and being able to easily add/remove keys.

If this was being designed from scratch though, I'd recommend:

Not using an abbreviation in the type name ("config")
The config type should be a class, not a struct, since it's pretty large (4 reference fields)
It should likely be immutable
In that case, those two dictionaries might've been two immutable dictionaries, which could've been assigned to a cached ReadOnlyDictionary<TKey, TValue instance to avoid extra allocations whenever there were no available keys.

Given we can't make breaking changes and the existing code though, changes here seems ok 🙂