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

Respect the configured serializer in Ui.Playground #1121

Closed

Conversation

jp2masa
Copy link

@jp2masa jp2masa commented Feb 16, 2024

This should allow using AOT with Ui.Playground, but adds an extra dependency to GraphQL.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ac38114) 93.88% compared to head (b190e13) 93.88%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1121   +/-   ##
=======================================
  Coverage   93.88%   93.88%           
=======================================
  Files          49       49           
  Lines        2370     2370           
  Branches      421      421           
=======================================
  Hits         2225     2225           
  Misses        103      103           
  Partials       42       42           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Shane32
Copy link
Member

Shane32 commented Feb 17, 2024

It seems that the purpose of this PR is either to (a) reduce dependencies, or (b) reduce code used in AOT scenarios. I don't think this PR really fulfills the intended purpose.

For ASP.NET Core 2.x, Ui.Playground uses the Newtonsoft converter that ASP.NET Core directly references. Utilizing the GraphQL serializer won't reduce dependencies and AOT isn't applicable for this version of ASP.NET Core.

Newer versions of ASP.NET Core rely on versions of .NET that include System.Text.Json. So if the purpose is to reduce dependencies, then this doesn't help. Newtonsoft.Json isn't designed for AOT, but ignoring this, you could say that using the same serializer as GraphQL.NET would allow System.Text.Json to be trimmed. But this argument doesn't hold when you look at the PR: the analyzer is going to see that System.Text.Json will be used if GetService returns null, and so will need to include STJ in any case. Futher, adding a dependency on GraphQL.NET is going to increase the size of any application that only uses the UI project.

There is another possible reason to utilize the configured serializer, and that is if we wanted to allow the user to customize the serialization process. But the serialization format in this case is known and fixed -- for instance, it (probably) wouldn't work if the serializer was configured for pascal case.

If you want to support AOT or other serializers, I suggest that you make a protected method to handle serialization. Then in your own implementation you can override the method and rewrite the serialization as desired. For AOT/trimming, the analyzer will see that the base method implementation is never referenced, and will be able to trim out the default serializer. Then if you wish to use the GraphQL.NET serializer, then it should be relatively easy to make a derived class, pull the serializer from DI, and use that within the protected method.

Another idea specifically for NativeAOT in .NET 7 and 8 would be add .NET 7 TFM that uses source generation to eliminate reflection.

Comment on lines +67 to +76
if (_services.GetService(typeof(IGraphQLSerializer)) is IGraphQLSerializer serializer)
{
using var stream = new MemoryStream();
serializer.WriteAsync(stream, value).Wait();

stream.Position = 0;

using var reader = new StreamReader(stream, Encoding.UTF8);
return reader.ReadToEnd();
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, GraphQL.NET provides a synchronous method as well:

Suggested change
if (_services.GetService(typeof(IGraphQLSerializer)) is IGraphQLSerializer serializer)
{
using var stream = new MemoryStream();
serializer.WriteAsync(stream, value).Wait();
stream.Position = 0;
using var reader = new StreamReader(stream, Encoding.UTF8);
return reader.ReadToEnd();
}
var serializer = _services.GetService<IGraphQLTextSerializer>();
if (serializer != null)
{
return serializer.Serialize(value);
}

@Shane32
Copy link
Member

Shane32 commented Feb 17, 2024

See #1124 for a PR to provide NativeAOT compatibility for this project.

@jp2masa
Copy link
Author

jp2masa commented Feb 17, 2024

See #1124 for a PR to provide NativeAOT compatibility for this project.

I think that would work for me. My original idea was to use the registered serializer because, in my case, it's a System.Text.Json serializer with a source generated JsonSerializerContext.

But the approach in #1124 looks better, so I'm closing this.

@jp2masa jp2masa closed this Feb 17, 2024
@jp2masa jp2masa deleted the ui-playground-serializer branch February 17, 2024 21:11
@Shane32
Copy link
Member

Shane32 commented Feb 19, 2024

v7.7.1 has been published containing #1124

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants