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

[Feature] Figure out how to isolate against Stable/Beta End-points to Microsoft.Graph package #52

Closed
michael-hawker opened this issue Jun 23, 2020 · 9 comments
Assignees
Labels
Completed 🔥 question ❔ Further information is requested
Milestone

Comments

@michael-hawker
Copy link
Member

Describe the problem this feature would solve

Unlike in JavaScript, C# can only reference one end-point or another (stable/beta) not both. We should figure out if there's an easy way for us to target/guard against one or the other to let that choice be in our developer's hands rather than ours.

Describe the solution

???

@ghost ghost added the needs triage 🔍 label Jun 23, 2020
@ghost
Copy link

ghost commented Jun 23, 2020

Hello michael-hawker, thank you for opening an issue with us!

I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌

@shweaver-MSFT
Copy link
Member

I did a little investigation. @nmetulev had a great idea to add a new csproj file for the CommunityToolkit.Net.Graph package and share the files between the two csprojs (one for beta and the other for V1 SDKs). I tried this out and I found that there are some minor, yet troublesome, differences between the types in each SDK. They are not perfectly aligned, and I suppose I shouldn't really expect them to be.

This is mainly a problem because as the SDKs diverge we need some sort of middle layer to mitigate the differences. I'm not sure we want to be in the business of maintaining a Graph SDK delta layer just so we can toggle between Graph endpoints.

In light of all this, I propose we duplicate the CommunityToolkit.Net.Graph project and specialize it for the Beta endpoint. The Controls package can be built on the V1/production Graph package, and the Beta package can be there for people to take advantage of as they choose to. I do NOT propose that we duplicate the controls project. as that seems overkill. In the future, if we do have any specific needs for Beta controls, they ought to go in a new CommunityToolkit.Uwp.Graph.Controls.Beta project.

Thoughts?

@michael-hawker
Copy link
Member Author

michael-hawker commented Apr 5, 2021

Haha, my thought from the weekend that I just put in #88 is along these lines @shweaver-MSFT... 😊 Just wanted to write it down before I forgot.

I think since the controls need limited calls to the Graph, if we make a common set for them (which I think is kind of how MGT does it), we could just provide two middle packages (or more) that provide implementations, one at least for Beta vs. V1 or we could even have a more light-weight HTTP type version which maybe makes it easier to toggle between them?

@nmetulev
Copy link
Contributor

nmetulev commented Apr 8, 2021

@shweaver-MSFT, I think that's a good compromise, I like it. I can only think of the login component needing beta (for person image on personal accounts) - we could do special cases for those by making one of beta calls when we need to, to avoid taking a dependency on the beta sdk. It's not clear though - if a developer wants to use the beta sdk and the controls, would they then need to reference both sdks?

Along the same thoughts as @michael-hawker, I wish the .NET Graph sdk exposed a smaller package of interfaces/simple client that we could use and allow developers to plug in whatever sdk they want? I wonder if the graph core package can be used like that - and we build our own calls to the graph as we need them for internal uses by the controls/helpers.

@shweaver-MSFT
Copy link
Member

@nmetulev, we can definitely do a special case for the person image (really wish that would make it to V1!!!).

With the setup I propose, developers can reference the controls package to use controls built on V1. To access Beta APIs, they can reference the Beta package. There is no option for combo beta/V1 controls at the moment. If we have controls in the future that use the Beta API in a meaningful way, they should go in their own *.Graph.Beta.Controls package.

In my mind, the V1 and Beta APIs can be treated as entirely different services. They have a lot of commonality at the moment, but not in a way we can rely on long term. Because of this, I'm hesitant to enable V1 controls to talk to the Beta endpoint at will, because the endpoints are ultimately not the same and assuming consistency could yield unexpected behavior.

The GraphPresenter is an interesting in-between, because it doesn't actually reference any of the Graph SDK types that can't be found in the Graph Core package. We can probably make a special case and enable a version property there because it makes Graph requests manually.

@nmetulev
Copy link
Contributor

nmetulev commented Apr 9, 2021

Agreed - I don't think we should do both. I'm just wondering if we could avoid using either one and instead allow all controls to follow a similar pattern to the GraphPresenter and make the calls without the v1 or beta sdk (but still only use v1 apis)? The idea being to avoid taking a dependency on either sdk.

@shweaver-MSFT
Copy link
Member

Ya, we talked about this directly. I'll investigate some more and see what it takes to remove the Graph SDK references

@shweaver-MSFT
Copy link
Member

shweaver-MSFT commented Apr 12, 2021

So there is an interesting side effect I didn't think of at first. Without the SDK, none of the controls can expose formal Graph types. For example, LoginButton has a UserDetails property which returns a Graph User type, PersonView has a PersonDetails property that returns a Graph Person type. Stripping out the Graph SDK means that developers will need to work with our middle layer types instead of the real ones, which could be a pain because even if we expose the underlying object value for Person/User, the dev will need to cast back to the proper Graph type and version and hope for the best.

I like the safety of the types and the dev experience we can offer down stream. I'm not sure removing the Graph SDKs altogether is the right thing to do. Removing the SDKs makes our code much more complicated. The bigger problem I see is how some of these most basic features (user photo, presence) haven't made their way to V1 yet. It puts pressure on us to make tough decisions about our future roadmap. But cutting out the SDK means the team won't get much feedback from us going forward, and they won't have us as a funnel for traffic. I'm quite hesitant to remove the SDKs.

@michael-hawker
Copy link
Member Author

@shweaver-MSFT I know we talked a bit more about this today. I liked @nmetulev of having at least the Login component abstracted to be simpler and just do the couple of calls we need without the SDK. We could still expose the 'Id' properties or such on the control itself, so if someone also uses the GraphSDK there's a way to index on it without them also having to duplicate the initial calls?

@ghost ghost locked as resolved and limited conversation to collaborators May 20, 2021
@shweaver-MSFT shweaver-MSFT modified the milestones: 7.0.0, 7.1.0 Jul 27, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Completed 🔥 question ❔ Further information is requested
Projects
None yet
Development

No branches or pull requests

3 participants