-
Notifications
You must be signed in to change notification settings - Fork 157
Remove graph api sdk dependency #1003
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
Conversation
var subscriptionModel = MapGraphEntityToModel(subscription); | ||
return subscriptionModel; | ||
}).GetAwaiter().GetResult(); | ||
var response = JToken.Parse(responseAsString); |
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.
Converting string to Model seems like it could be an extension method?
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.
Also tagging @KoenZomers as he wrote a lot of the original code
var subscriptionModel = MapGraphEntityToModel(subscription); | ||
return subscriptionModel; | ||
}).GetAwaiter().GetResult(); | ||
var response = JToken.Parse(responseAsString); |
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.
@quails4Eva : would recommend using System.Text.Json over Newtonsoft
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.
I think I've done this. I copied the newtonsoft code from elsewhere in the project, but I think this should be equivalent for system.text.json . It needed the package version updating for .net standard. I've changed it to use the same version for each target framework as I don't think system.text.json is tied to framework versions as some other packages are
Next time I have some free time to look at this I'll try and replace the other usages in SubscriptionsUtility |
I started adding some async http bits to maintain where async was already being used, but then I realised that the async usage was all wrapped in Task.Run so probably not really helping anything. I can undo that if you'd rather it be put back. |
I've just used the System.Text.Json ability to store overflow properties in a dictionary, they'll be whatever type that uses. It's still a breaking change as the old version could return objects from the Graph Sdk, but not much that can be done about that when we're removing Graph Sdk. |
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.
MSGraph package reference is still in here, is this intended?
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.
No, that was an oversight from me. I've removed it now.
I also removed Microsoft.Graph.Core and it still builds, though I can add that back in if it causes issues.
We were able to identify the issue causing this, I did a wrong reference of this PR my code. Sorry for the false alarm. Cheers |
So, when will this fix / update be available as PnP Nuget package? Checked today, Graph client is still included. |
@jansenbe @matijahanzic anything in particular that's needed to progress this pr? I don't have a great way to test the changes as my projects using PNP don't use these bits of the library. Most of my testing was using sample graph API responses and checking that it was converting to objects correctly. |
It would be really helpful to merge this PR. I think it may resolve the long-standing problems when using Microsoft.Graph modules alongside PnP.PowerShell (a common requirement!) |
From my end, this looks good. I hope to see this merged soon. We also have issues using Microsoft Graph and PnP in our projects. |
Hi @quails4Eva, @jansenbe can we help with the testing? Or do you need any other help to speed up this merge? :) |
I'm not familiar with the testing process/requirements for this repo, but I assume if you're able to test using this branch that would increase the confidence that the changes haven't broken anything. |
How's it looking? Sure hope this is released soon! Still running PnP 1.12.0 due to the conflict with MgGraph. Really need some of the new features in the 2.x.0 release of PnP. Please release soon!! :) |
Guessing this didn't make it in PnP 3.0.0? Does anyone know? |
@quails4Eva / @Tanddant : I today took a look at this PR and tried to build it which failed due to the System.Text.Json.Nodes namespace not being available in the system.text.json version available for .NET Standard. Ideally the JSON handling for this PR needs be rewritten without needing the JsonNode class, alternative is using compiler directives to add an alternate code path for the .NET Standard distribution, but that will result in more code to maintain going forward. I've added the System.Text.Json references like this: <ItemGroup Condition=" '$(TargetFramework)' == 'netstandard2.0' ">
...
<PackageReference Include="System.Text.Json" Version="4.7.2" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' == 'net8.0' ">
...
<PackageReference Include="System.Text.Json" Version="8.0.5" />
</ItemGroup>
<ItemGroup Condition=" '$(TargetFramework)' == 'net9.0' ">
...
<PackageReference Include="System.Text.Json" Version="9.0.0" />
</ItemGroup>
|
@jansenbe is there a reason the .net standard package is targeting such an old version? The latest stj (System.text.json) supports .net standard. I'm happy to be corrected, but I think that it should have one of the following.
I tried to find info on which of the above is best practice but only found dotnet/runtime#105014 which has some mixed messages. |
Also found this dotnet/runtime#110265 (comment) which suggests doing something like my suggestion 1, though I don't know how authoritative the answer is. |
Think you're right @quails4Eva , seems there's a v9 version that's compatible with .NET framework. I'll do some testing to bump this in PnP Core SDK and PnP Framework. Once that works this PR can be probably be merged |
Reduce dependency on Microsoft.Graph NuGet package
Based on discussion in #660
#660 (comment)