workos / workos-dotnet

Official .NET SDK for interacting with the WorkOS API
MIT License
11 stars 8 forks source link

Update project file to target net5.0, 6.0, 7.0 #134

Closed pliao28 closed 1 year ago

pliao28 commented 1 year ago

Description

Add Targetframeworks for net 5.0, 6.0, and 7.0

Documentation

Does this require changes to the WorkOS Docs? E.g. the API Reference or code snippets need updates.

[ ] Yes

If yes, link a related docs PR and add a docs maintainer as a reviewer. Their approval is required.

maxdeviant commented 1 year ago

@pliao28 Could you expand on why this is necessary? What are we trying to achieve here?

maxdeviant commented 1 year ago

@pliao28 Could you expand on why this is necessary? What are we trying to achieve here?

Based on this article I think we should probably just keep targeting .NET Standard 2.0.

This will allow us to retain compatibility with both .NET 5+ and .NET Framework.

When we do want to drop support for .NET Framework, we can target .NET Standard 2.1.

linear[bot] commented 1 year ago

USRLD-792 Add net5.0,6.0,7.0 compilation targets

pliao28 commented 1 year ago

@pliao28 Could you expand on why this is necessary? What are we trying to achieve here?

@maxdeviant , I don't see any downside to adding the newer versions to multi-target as we are not using anything that was deprecated between versions that requires TFM-dependent code. We should have frameworks targeted for currently supported frameworks (6.0 and 7.0) as well as any previous EoL frameworks that people still may use. For anyone new to touching the .net SDK they will likely run into a versioning error having a newer version of NET installed like net6.0 .

It's also just a step for the future to allow support using newer .net features such as using system.text.json over newtonsoft.json. (This will require TFM-dependent code)

Looking at Stripe's dotnet sdk they currently added up to 6.0 https://github.com/stripe/stripe-dotnet/blob/master/src/Stripe.net/Stripe.net.csproj

maxdeviant commented 1 year ago

@pliao28 Could you expand on why this is necessary? What are we trying to achieve here?

@maxdeviant , I don't see any downside to adding the newer versions to multi-target as we are not using anything that was deprecated between versions that requires TFM-dependent code.

Multi-targeting adds a lot of overhead in having to remember which features can be used for which targets.

If we're building against .NET Standard 2.0 we have a consistent base for everything, so we should stick to that unless there are features we cannot use without targeting .NET Standard 2.0.

We should have frameworks targeted for currently supported frameworks (6.0 and 7.0) as well as any previous EoL frameworks that people still may use.

By targeting .NET Standard 2.0 we are ensuring that we aren't using features that are deprecated. We don't need to target individual .NET versions the same way we do with other SDKs that depend solely on different language versions.

For anyone new to touching the .net SDK they will likely run into a versioning error having a newer version of NET installed like net6.0 .

This is something that will be solved by Devbox, so we shouldn't have to change our targeting to support this.

We will want to use the latest mainline version of .NET for our own development, at some point.

It's also just a step for the future to allow support using newer .net features such as using system.text.json over newtonsoft.json. (This will require TFM-dependent code)

Looking at Stripe's dotnet sdk they currently added up to 6.0 https://github.com/stripe/stripe-dotnet/blob/master/src/Stripe.net/Stripe.net.csproj

I think switching to System.Text.Json is more than we want to do right now (and almost certainly requires dropping support for .NET Framework, which I don't know if we want to commit to just yet).

We're likely better off just keeping on .NET Standard 2.0, like we have been.

pliao28 commented 1 year ago

@maxdeviant got it, I haven't been up to date on devbox, figured this would be something simple to add to avoid the versioning errors that I got while running them. Closing PR.