nozzlegear / ShopifySharp

ShopifySharp is a .NET library that helps developers easily authenticate with and manage Shopify stores.
https://nozzlegear.com/shopify-development-handbook
MIT License
747 stars 309 forks source link

.Net standard support #926

Closed clement911 closed 9 months ago

clement911 commented 1 year ago

@nozzlegear , any thoughts on incrementing the major version and targeting only .NET 6+?

That would allow us to use the latest c# features, .net types and runtime features.

I have a PR to include a generated file with all the graphql classes, automatically generated, but it requires .NET6+ because it needs System.DateOnly.

clement911 commented 1 year ago

Speaking of, I think you had used some new c# feature in your recent commit https://github.com/nozzlegear/ShopifySharp/commit/1f1ccd1a13b7beb82f8afaa4279877af5babd3d1 , which broke the build.

I also would like to use these new features 😀

nozzlegear commented 1 year ago

I’d love to update to .NET 6 and use all of the new C# stuff! That recent commit was me playing around with adding them, I have a local branch where I was looking into maybe using conditional targeting (#if NET60 kind of stuff) but it’s been kind of a pain to get it working nicely so far. I think I just don’t have enough experience with conditional targeting at the moment.

I’ll have to think about dropping support for .NET Framework. I don’t really want to do it because people are still actively using it, but at the same time I really want to add more modern stuff to ShopifySharp that would make it much more usable for the rest of us who can target .NET 6, 7 and 8. I’d love to merge your GraphQL changes in particular, as I’d been looking at frameworks like Chili Cream and incorporating them into ShopifySharp for better GraphQL support.

Maybe it’d be possible to have a separate ShopifySharp.DotNetFramework package that maintains or re-adds support for DNF, while the main package drops it. I’ll think about this, open to feedback though if you have any!

-- Joshua Harms

On Sep 13, 2023, at 19:01, Clement Gutel @.***> wrote:

Speaking of, I think you had used some new c# feature in your recent commit 1f1ccd1 , which broke the build. I also would like to use these new features 😀 — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

clement911 commented 1 year ago

I don't have a ton of experience in that area either. Is there a way to see the percentage of nuget installs that targets .Net framework?

The change I have is similar in spirit to what Chili Cream would get you (if I understand Chili Cream). It's just a few lines of code to query the graphl model via introspections and generate POCO classes.

nozzlegear commented 1 year ago

That’s a good question, I’ll check Nuget tomorrow and see if there are stats about .Net Framework. For the GraphQL stuff, your change sounds great! I’d much rather do something like that instead of bringing in another dependency.

-- Joshua Harms

On Sep 13, 2023, at 21:27, Clement Gutel @.***> wrote:

I don't have a ton of experience in that area either. Is there a way to see the percentage of nuget installs that targets .Net framework? The change I have is similar in spirit to what Chili Cream would get you (if I understand Chili Cream). It's just a few lines of code to query the graphl model via introspections and generate POCO classes. — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

clement911 commented 1 year ago

FYI This is what the generated schema file looks like: https://github.com/Wish-Org/Wish.GraphQLSchemaGenerator/blob/main/Wish.GraphQLSchemaGenerator.Tests/shopify.cs

It makes it much easier to query grapqhl.

It's worth noting that the generated schema uses System.Text.Json and not Newtonsoft.

I've already updated ShopifySharp to be able to use System.Text.Json when using the GraphService.

However, we cannot include the generated shopify.cs above in ShopifySharp because it contains .NET 6 features such as System.DateOnly and interface default implementations 😢

clement911 commented 1 year ago

I think I figured it out 🙂

I've added a target framework for .net6.0 specifically and added a pre-compile directive to only include the generated GraphQL types for .net6 or greater.

https://github.com/nozzlegear/ShopifySharp/blob/d139d780d572e06e68bc415a2cfdcb99e1e45aff/ShopifySharp/ShopifySharp.csproj#L3

https://github.com/nozzlegear/ShopifySharp/blob/d139d780d572e06e68bc415a2cfdcb99e1e45aff/ShopifySharp/Entities/GraphQL/GraphQLSchema.generated.cs#L1

Here is an example on how to use the generated graphql types to query grapqhl with strongly typed objects:

https://github.com/nozzlegear/ShopifySharp/blob/d139d780d572e06e68bc415a2cfdcb99e1e45aff/ShopifySharp.Tests/GenerateGraphQLSchema_Test.cs#L53-L79

clement911 commented 1 year ago

I'm going to leave open though, as using pre-compile directive is not great.

clement911 commented 1 year ago

@nozzlegear ready for you to approve: https://github.com/nozzlegear/ShopifySharp/actions/runs/6182839771

clement911 commented 1 year ago

@nozzlegear I know I made a few important changes. Let me know if you have any concern.

nozzlegear commented 1 year ago

I haven't had a chance to look at it yet, but I'm planning to review this weekend! Don't be afraid to poke me if it seems like I forgot though, I'm a bit of a space case :P

clement911 commented 1 year ago

Haha no worries. Thanks 👍

nozzlegear commented 1 year ago

Haven’t forgotten about this! Finishing up a big sprint for a client tonight/tomorrow and then I’m going to be putting some work into ShopifySharp.

-- Joshua Harms

On Sat, Sep 16, 2023, at 01:35, Clement Gutel wrote:

Haha no worries. Thanks 👍

— Reply to this email directly, view it on GitHub https://github.com/nozzlegear/ShopifySharp/issues/926#issuecomment-1722152910, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASOE7C4ID6GBWU77XBEUVTX2VCERANCNFSM6AAAAAA4UK6RIM. You are receiving this because you were mentioned.Message ID: @.***>

clement911 commented 1 year ago

No problem. There is no rush by the way so take your time

clement911 commented 1 year ago

@nozzlegear seems like you got busy. Any chance you could review the pending release deployment?

nozzlegear commented 1 year ago

Ah yes! I’ll take a look. I have a couple small things I need to add to the package for a client as well.

-- Joshua Harms

On Oct 2, 2023, at 05:45, Clement Gutel @.***> wrote:

@nozzlegear seems like you got busy. Any chance you could review the pending release deployment? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

clement911 commented 1 year ago

I needed the latest lib so I've just pulled the pre-release nuget package. It's in production already. No issues so far.

nozzlegear commented 1 year ago

Okay, good to hear! I don’t have any calls scheduled tomorrow so I’m scheduling ShopifySharp work for myself instead 😛 I have lots of ideas I want to experiment with, not to mention the changes you’ve made for GraphQL.

-- Joshua Harms

On Wed, Oct 4, 2023, at 17:16, Clement Gutel wrote:

I needed the latest lib so I've just pulled the pre-release nuget package. It's in production already. No issues so far.

— Reply to this email directly, view it on GitHub https://github.com/nozzlegear/ShopifySharp/issues/926#issuecomment-1747724264, or unsubscribe https://github.com/notifications/unsubscribe-auth/AASOE7BKGFZZ5GMTMDIHFUTX5XN5XAVCNFSM6AAAAAA4UK6RIOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONBXG4ZDIMRWGQ. You are receiving this because you were mentioned.Message ID: @.***>

clement911 commented 1 year ago

I fixed another small bug and queued a new release to be reviewed. https://github.com/nozzlegear/ShopifySharp/actions/runs/6413991536

nozzlegear commented 9 months ago

I'm going to close this for now I think. I'm not sure what (if anything) has changed, but we're now able to use C# 12 in the project records, file-scoped namespaces, and #nullable enable which previously wasn't available to netstandard2.0. It all builds just fine with no warnings, and the automated tests I have for .NET Framework 4.7.2 have no problems either.

The only thing that I've tried to use which I wasn't able to was System.Buffers (needs .NET 7.0 or greater) and the required keyword (needs .NET 8.0). I'm happy with using the preprocessor directives to add those as little "nice to have enhancements" for people using .NET 7/8, like I did with the ShopifyRequestValidationUtility.