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

Make a serialization object for JS interops #5701

Open
stsrki opened this issue Aug 18, 2024 · 9 comments · May be fixed by #5827
Open

Make a serialization object for JS interops #5701

stsrki opened this issue Aug 18, 2024 · 9 comments · May be fixed by #5827
Assignees
Milestone

Comments

@stsrki
Copy link
Collaborator

stsrki commented Aug 18, 2024

          > > > Make a serialization object with the first letter in lowercase. This ensures that we get appropriately named JSON objects in case users have different JSON serialization settings. Unlikely but you never know.

You touch on an important point here. We should not rely on anything external or conventions. We should precisely define how our own stuff gets serialized so it can't fail due to external conditions. Is this not possible?

Well, we could introduce internal serialization class objects that make use of JsonPropertyNameAttribute to strongly type the name of the field. That way, it would be 100% right.

Yes. If we care about it. That's a way to go I guess, not doing guess work and hoping nothing external happens in my opinion.

Originally posted by @David-Moreira in #5691 (comment)

@stsrki stsrki added this to the 1.7 milestone Nov 1, 2024
@tesar-tech
Copy link
Collaborator

I am not sure I understand the issue correctly.

I guess it's about the Source/Blazorise/Components/Tooltip/Tooltip.razor.cs form the pr where you changed the PascalCase to camelCase so the tooltip library digest that correctly?

And you want to make sure it gets serialized like that (in camelCase ) no matter the externalities?

Can you describe the externalities?

To my understanding the camelCasing is already there. When the object arrives to js, all the propertyNames are already in camesCase and you really need to try hard to change that. There is no magic setting in System.Text.Json that can change that globally and this convention will hardly change...

Or did I misunderstood the issue completely @stsrki ?

@David-Moreira
Copy link
Contributor

There is no magic setting in System.Text.Json that can change that globally and this convention will hardly change...

@tesar-tech if I remember correctly the point is that the serializer could be configured in a global way that could affect any convention we might rely on. So we shouldn't rely on conventions that are susceptible to be changed, if they can be that is.
Is it not possible to configure the serializer in a global way? i.e change from camelcase convention to pascal case? I was under the impression it was possible.

@tesar-tech
Copy link
Collaborator

tesar-tech commented Nov 4, 2024

@David-Moreira It is not possible dotnet/runtime#31094 and also marked as Not planned.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 4, 2024

So far, when we serialized the object to JS, we relied mostly on the built-in JSON serializer, assuming everyone would have camelCase settings. But assuming it is not ideal and we need to handle it ourselves.

@tesar-tech The Tooltip you mentioned was just the first iteration of trying to solve the problem.

So, the plan is to have a dedicated serialization object with all the field names properly named using the JSON name attribute. We have already made it on the Video component. See:


We basically need to look everywhere where we have a new object for serializing to JSON and implement a new serialization class.

@tesar-tech
Copy link
Collaborator

Oh, yeah. You mean making the object that goes to js a strong type... I agree with that.

But I don't see a reason why to include the JsonPropertyName attribute - as discussed above. I would do it only for those properties that require something else than the default camelCase...

Also maybe records instead of class to make it more condensed? Or at least use require keyword to force all the important stuff to be declared..? But that's probably for a different discussion or pr...

So what is left here? The JsOptions for tooltip?

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 4, 2024

We want this make sure everything is handled properly. So all file must have JsonPropertyName.

Tooltip also need to have a new object.

As for what kind of. Stick with the class. As we have it for Video options.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 6, 2024

I think Blazor is always serializing to Camelcase.

Also, I found this comment from a Blazor team member saying that they don't have plans to change it as it could affect Blazor itself. dotnet/aspnetcore#38144

So we might not need to do this at all.

@David-Moreira
Copy link
Contributor

@stsrki But that's precisely what @tesar-tech was saying. There's currently no way to configure the options globally. I personally was under the impression it was, since it was implied so before.

I'd still take concrete objects over anonymous objects as an improvement to the code base.

@stsrki
Copy link
Collaborator Author

stsrki commented Nov 6, 2024

@stsrki But that's precisely what @tesar-tech was saying. There's currently no way to configure the options globally. I personally was under the impression it was, since it was implied so before.

I was also under such an impression. But I never tried it in the past.

I'd still take concrete objects over anonymous objects as an improvement to the code base.

Agree. At least make concrete objects even though we might not need to have JsonPropertyName attribute.

@tesar-tech tesar-tech linked a pull request Nov 7, 2024 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No Status
Development

Successfully merging a pull request may close this issue.

3 participants