Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Blazor] Use JSON source generator during WebAssembly startup #54956
[Blazor] Use JSON source generator during WebAssembly startup #54956
Changes from 11 commits
a42088e
dfb6f93
11c518c
0eb1454
24335bc
54f2b82
361c91c
b06b3c4
17f9810
d758ef8
04910bb
75a71f6
35c4f33
03bdce3
fb95f5c
2996e5c
a10f5c0
48e7961
9a53261
f08d858
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Why does this warning happen? I can't see any usage of
IgnoreNullValues
in this PR.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.
This warning was coming from generated code, but it looks like that's not happening anymore for some reason. I've just removed this.
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.
How does the
jsonOptions
parameter defined in line 34 factor into the configuration of this new JSO instance?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.
That primarily gets used to deserialize event args from JavaScript events. If we reused
jsonOptions
rather than creating a new one specifically for the call toInvokeVoidAsync()
at the end of this method, we'd have to add aDefaultJsonTypeInfoResolver
at the end of theTypeInfoResolverChain
. That's because developers can dynamically register their own customEventArgs
types that we later deserialize when that event gets emitted from JS.However, if I do that, then startup time explodes, because
ResolvePolymorphicConverter
ends up initializing aJsonTypeInfo
for each of the interfaces that each of the arguments toIJSRuntime.InvokeVoidAsync()
implements. For example, all the interfaces thatSystem.Int32
implements get their ownJsonTypeInfo
because we pass anint
in the call toInvokeVoidAsync()
.This is probably due to the fact that
JSRuntime
serializes arguments as anobject[]
. I worked around this in an earlier prototype by instead serializing each argument individually, using.GetType()
as theinputType
argument inJsonSerializer.Serialize()
. But that felt like a hack. Maybe you know of a better way to avoid this pitfall?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 see this as another reason to deprecate the
object[]
overloads and give each argument a generic parameter.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.
Is
WebRenderer
instantiated as a singleton? If not, creating a new JSO for every instance is bound to create initialization overheads.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.
It's scoped, not singleton. The scope is per HTTP request for SSR, or per WebSocket connection for Blazor Server.
It might be possible to share a JSO across scopes as long as there's no risk of interference or information leakage across users when doing that.
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.
That shouldn't be an issue unless user info is encapsulated by any custom converters.
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.
Actually you reminded me that the custom converters very much do require access to scoped context. For example see
DotNetObjectReferenceJsonConverter
which has to access the scope'sJSRuntime
in order to keep track of the references on a per-scope basis.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.
Yeah, I think we've had this discussion before and it seems to be an unfortunate consequence of STJ not supporting DI that is scoped to a particular serialization operation (tracking issue). Is there any way we could decouple the converters from user state using APIs we have in our disposal today? Perhaps
AsyncLocal<T>
?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.
Given the degree of antipathy the team has towards
AsyncLocal
in general, and the likely back-compat problems with such a change, it's unlikely we'd go that way I'm afraid!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'm curious about how you think we should maintain this in the future. How would a developer know when/what to add to this list of types? Am I right to think that if they start serializing something different and fail to add it here, everything would still work but would just be slower? Or would some error occur so they know to change this code?
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.
Along the same lines, how did you even know to include these specific types? I'm hoping it's because if you don't, there's a clear error indicating they need to be included!
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.
These types match the types of the arguments passed to the JS interop call. Failing to do this results in a runtime error. However, I've just pushed a change that makes it even clearer where these types come from, so hopefully that eliminates any confusion. If we end up reverting that change, I can add a comment explaining how these types should be specified.
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.
Depending on how
WebAssemblyComponentSerializationSettings.JsonSerializationOption
is defined, you might be able to avoid creating this intermediate options instance by configuring yourJsonSerializerContext
via theJsonSourceGenerationsOptionsAttribute
.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.
Is this going to work when we have to deserialize the concrete parameters?
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.
You'd need to additionally register the types you expect
object
to be. Given that @MackinnonBuck is planning on adding the reflection-based resolver as a fallback it would work even if the list isn't exhaustive, but making sure the 80% is handled by the source generator should contribute to improved startup times.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.
Since this
JsonSerializerContext
is only used during deserialization, we happen to know that theobject
inIList<object>
will either be aJsonElement
ornull
. Why we didn't initially choose to deserialize aJsonElement[]
directly, I'm not sure.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.
Why not call
Deserialize<IList<JsonElement>>
(orDeserialize<IList<JsonElement?>>
if null is valid) and register[JsonSerializable(typeof(IList<JsonElement?>))]
here?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 would strongly recommend creating one central
JsonSerializerContext
in your project that declares all needed types in one place. This should serve to improve both application size and startup time. Even though these might generate for different types superficially, transitively there is bound to be overlap (e.g.JsonTypeInfo<int>
,JsonTypeInfo<string>
but also shared internal types) and it would be best if these were only generated/initialized at a single location.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 agree this would be ideal - Unfortunately, I haven't found a good way to achieve that in totality. The types that we serialize are split across multiple assemblies and shared sources. I fixed the example highlighted here, but I don't think there's much else we can easily improve. Maybe one possible improvement is having the
DefaultAntiforgeryStateProvider
shared source use a duck-typedInternalJsonSerializerContext
that supportsAntiforgeryRequestToken
. So each project that referencesDefaultAntiforgeryStateProvider
must also define anInternalJsonSerializerContext
. But then we'd probably like to be sure that eachInternalJsonSerializerContext
has the sameJsonSourceGenerationOptions
configured... Hm..