Skip to content
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

Support for injecting WebTokenRequest.Properties #207

Merged
merged 5 commits into from
Apr 17, 2023

Conversation

Richasy
Copy link
Contributor

@Richasy Richasy commented Apr 14, 2023

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)
  • 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)
  • Contains NO breaking changes

Other information

@ghost
Copy link

ghost commented Apr 14, 2023

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 🙌

@Richasy Richasy changed the title Add V2 model for Uwp authorization Support for injecting WebTokenRequest.Properties Apr 14, 2023
Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of thoughts based on our recent learnings with nullability annotations.

Though also meant to ask @Sergio0694 about ramifications/overhead of empty initialized collection vs. null pointer in C#. In the other case I saw in our other toolkit code I wasn't worried about it as the collection was basically being initialized in the collection the majority of the time and would contain values, but here this isn't always going to be the case.

I mean, in general it's generally easier to work with an empty collection than a null value when it comes to enumerations.

@michael-hawker
Copy link
Member

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
Copy link
Member

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 🙂

Copy link
Member

@michael-hawker michael-hawker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM - Thanks!

@michael-hawker michael-hawker merged commit 9fc2a99 into CommunityToolkit:main Apr 17, 2023
@Richasy Richasy deleted the richasy/AddV2Support branch April 18, 2023 01:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants