search

graphql-dotnet / graphql-dotnet

GraphQL for .NET
https://graphql-dotnet.github.io
MIT License
5.83k stars 924 forks source link

Release v8.0 #4004

Closed Shane32 closed 1 week ago

Shane32 commented 1 month ago

What features should be completed before releasing GraphQL.NET v8 ?

See v8 milestone

Completed: (among others)

gao-artur commented 1 month ago

This one please! (server project)

Shane32 commented 1 month ago

I'm ready to release v8 any time now. The remaining features that I would want to add seem to require a lot of work:

The following feature would be nice, as it's part of the official GraphQL specification:

Reworking the field builders so they only have applicable methods on them would be a big benefit, I think, but with the analyzers provided by @gao-artur there is much less benefit than previously. See:

There's some misc tasks related to type-first in #2893 that could be done... most are not breaking changes though.

There's some dataloader misc tasks that could be done. Most are probably not breaking changes. The biggest question is if we should move the data loader classes into the main GraphQL nuget package. I don't know why they are separate; there are no additional dependencies in the dataloader package.

I'd really like to merge this code, but it needs work: (relates to #2923 or something)

So I don't really feel strongly motivated to work on any of the above tasks at the moment - lol !

Shane32 commented 1 month ago

This one please! (server project)

Ok we should come up with suggested changes to the API

gao-artur commented 1 month ago

I may need to implement this analyzer

depending on how much you want to implement this note from #3804

We could set ALL undefined properties to their default values, not just init-only ones, making reflection, source-generated and dynamically-compiled versions simpler.

It looks like a breaking change, so if not now, we will have to wait for v9. I have already tried implementing this analyzer, but InputGraphTypeAnalyzer is a big mess and will require refactoring to make the code extensible. With my limited free time now, it may take a long time.

gao-artur commented 1 month ago

There are a lot of obsolete API's we can remove now together with their analyzers and code fixes. I suggest adding a strong recommendation to the v8 migration notes to upgrade first to the latest v7 and apply all the automatic code fixes before upgrading to v8.

Shane32 commented 1 month ago

I suggest adding a strong recommendation to the v8 migration notes to upgrade first to the latest v7 and apply all the automatic code fixes before upgrading to v8.

Sure. Typically I've released the last minor version alongside the new major version, marking anything deprecated in the prior version that we can in order to help prepare for the new major version.

There are a lot of obsolete API's we can remove now together with their analyzers and code fixes.

I'm not sure there are any obsolete APIs that I would remove at this time. I just skimmed through all uses of [Obsolete] and they are mostly what I expected. I think we should consider marking GraphQLMetadataAttribute as obsolete. To do so, I need to better understand its use, ensuring that alternative attributes are available.

gao-artur commented 1 month ago

How about Field methods that return FieldType? They were marked as obsolete in v5, and we have automatic code fixes to rewrite all of them with the FieldBuilder methods.

Shane32 commented 1 month ago

I may need to implement this analyzer Ensure input object fields default don't conflict with source members default #3823 depending on how much you want to implement this note from #3804

I'd skip for v8. We don't have source generation yet anyway.

Shane32 commented 1 month ago

How about Field methods that return FieldType? They were marked as obsolete in v5, and we have automatic code fixes to rewrite all of them with the FieldBuilder methods.

So they were marked as obsolete for 2.5 years during all of v5 and v7? Do the code fixes work if the method is removed?

I was sorta hoping that we would have an entire version cycle (v8) for users to utilize the automatic code fixes. The analyzers have only existed since 7.7.0...

But it's been awhile as you say; we could remove them ...

gao-artur commented 1 month ago

Code fixes won't work because they need the analyzed code to compile. If there is something you still want to keep for v8, we can change the diagnostic level from warning to error (the user can change it back to warning with .editorconfig). This will nudge users to get rid of the obsolete code so it can be safely removed in v9.

Shane32 commented 1 month ago

Good idea, although FYI I'm more thinking about users who only ever upgrade every few major versions. We often were answering questions for people upgrading from v4 to v7, for example. So based on how often we received issues of this nature, and my own team's coding patterns, I've been making a point to attempt to keep the bulk of the graph types' code stable (to the point at which it compiles) as long as possible, only requiring code changes to infrastructure that's changed. (So I'm basically just talking about the Field methods.) Now with the analyzers it should be much easier for people to perform the upgrade, but that doesn't mean that people are going to upgrade very often. As the saying goes, if it ain't broke, don't fix it.

Maybe our rule of thumb going forward for Field-type methods is something like this:

That would indicate we should remove the methods marked as obsolete in v5, which we can do if you want.

Shane32 commented 1 month ago

Usually I just try to keep obsolete infrastructure code around for one major version, when reasonably feasible.

Shane32 commented 1 month ago

Or provide some kind of patch/reversion for one major version, like the legacy complexity analyzer.

gao-artur commented 1 month ago

We often were answering questions for people upgrading from v4 to v7

That's exactly why I suggested starting the migration notes from the recommendation to upgrade to the latest v7 first 🙂 I'd love to get rid of the obsolete code that sits there long enough.

Shane32 commented 3 weeks ago

@gao-artur What's next for 8.0? Are we ready to release? The only thing I'd really want to do is #3550, which requires changes to the connection builders/types so they are AOT compatible, which I also want to do, but it's going to be a lot of changes, and probably mostly breaking changes, so I was going to leave it alone for v8.

gao-artur commented 3 weeks ago

I'd just want to remove obsolete apis and ensure UI projects have support for OneOf input types

Shane32 commented 3 weeks ago

I'd just want to remove obsolete apis

ensure UI projects have support for OneOf input types

What are you thinking for this? Ensure our UI packages are up to date? It's still a proposed feature, not officially part of the spec, so they may not all support it yet. That being said, it's a backwards-compatible change (essentially it's just a marker directive to indicate that the server may reject certain requests) so all UI projects should work nicely with it regardless.

gao-artur commented 3 weeks ago

I remember you said UI uses the graphql.js package, and the support for OneOf was added in v16. I tried using the v7 version of Playground and GraphiQL, but they don't request oneOf fields from introspection. I tried to understand how the UI packed and how to upgrade it, but it looks like dark magic to me 😅

Shane32 commented 3 weeks ago

GraphQL Playground has not been supported for a couple years now; I tested GraphiQL 3.5.0 (latest release) and it doesn't appear to support oneOf, and I looked through the Altair changelog and don't see support there either. Do you want to look at this further?

gao-artur commented 3 weeks ago

No, it's ok. Let's releases it!

gao-artur commented 3 weeks ago

What about analyzers level for obsolete API? Do you want to change it to error?

Shane32 commented 3 weeks ago

Personally, since [Obsolete] defaults to warning level, I'd leave the analyzers for obsolete code at warning level also. Any obsolete methods still left in the code should execute correctly. What do you think? People can adjust the warning level pretty easily if they want to regardless.

Shane32 commented 3 weeks ago

If you're done with analyzers for 8.0, then I'll work on releasing 7.9 and 8.0 and updating the other projects (server, etc) over the next week. Not really sure how much time I'll have, so we'll see how it goes. Server should be ready to go, at least. I might do some organization on the migration doc if I have time also.

gao-artur commented 3 weeks ago

Personally, since [Obsolete] defaults to warning level, I'd leave the analyzers for obsolete code at warning level also. Any obsolete methods still left in the code should execute correctly. What do you think? People can adjust the warning level pretty easily if they want to regardless.

Fine with me.

gao-artur commented 3 weeks ago

If you're done with analyzers for 8.0, then I'll work on releasing 7.9 and 8.0 and updating the other projects (server, etc) over the next week. Not really sure how much time I'll have, so we'll see how it goes. Server should be ready to go, at least. I might do some organization on the migration doc if I have time also.

Nothing I see as must for v8. Just need to merge the shipped/unshipped files.

Shane32 commented 2 weeks ago

Version 8 is released! Probably need to update Conventions and the old Authorization/Relay packages also.

gao-artur commented 2 weeks ago

Great! Thank you!

Shane32 commented 2 weeks ago

@gao-artur I'd be interested to know your upgrade experience with v8. I've been pretty pleased on my end. My helper libraries needed to be updated due to the changes to IValidationRule but pretty easy otherwise.

gao-artur commented 2 weeks ago

Didn't start yet. Hope to upgrade next week

gao-artur commented 1 week ago

I must admit this was the smoothest upgrade in the history of GraphQL.NET! Very few irreversible breaking changes and zero unexpected runtime errors. Except two bugs (#4040 and #4042) I also encountered a false positive GQL010 diagnostic on the following input type

public class SystemFieldFilterGraphType : InputObjectGraphType<IList<SystemFieldExpressionParam>>

This graph type is generated from the DB configuration and has a registration in ValueConverter for IList<SystemFieldExpressionParam>. So, it worked well in runtime, but the static analyzer reported an error. I think this is a minor use case, and there is no need to fix the diagnostic. Users can suppress this diagnostic with #pragma warning disable GQL010, or, if they have many overrides in ValueConverter, set the severity to none in .editorconfig.

Also, moving the DataLoader's ResolveAsync extensions to GraphQL namespace didn't bring the expected benefit. Because the GraphQL namespace is usually not imported, these extension methods still have poor discoverability. The only using that is certainly imported is GraphQL.Types, where the ObjectGraphType, InputObjectGraphType, and InterfaceGraphType are defined. Maybe we should consider moving them into GraphQL.Types in the next major version.

The only thing I would like to ask before releasing 8.0.2 is adding argument type validation in PatternMatchingVisitor because currently, it silently ignores the directive if the type is not string. I thing it should throw an exception.

Shane32 commented 1 week ago

The only thing I would like to ask before releasing 8.0.2 is adding argument type validation in PatternMatchingVisitor because currently, it silently ignores the directive if the type is not string. I thing it should throw an exception.

Seems reasonable. Want to create a PR?

gao-artur commented 1 week ago

Sure

gao-artur commented 1 week ago