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

Refactor: add strongly typed serialization objects #5827

Merged
merged 20 commits into from
Nov 13, 2024

Conversation

tesar-tech
Copy link
Collaborator

Description

Draft for strongly-typed options, currently focused on the button and color picker components. I need your feedback to continue further.

Closes #5701

Key Points

  • Type Changes: Updated from object to TheComponentJsOptions in JsModule and its interface, as well as when calling methods in the module (typically within TheComponent.razor.cs).
  • Using record Types: Given that we're targeting .NET 6, the required keyword isn’t available. Using record types is a suitable alternative, as it allows for clean initialization without needing SomeProperty = SomeProperty declarations. This makes both the definition and initialization more concise. It can be of course done the "old way", but I don't see any advantage there.
  • File Organization: The JsOptions files are organized within a JsOptions folder. I followed the codebase conventions, placing everything within the Blazorise namespace.

Questions

  • Interfaces for JsModules: I’m unclear on the purpose of having interfaces for JsModules, especially since each one seems to have only one implementation without further usage. It feels like an extra step when working with these modules. (Is there a particular rationale here?)

Future Improvements

  • Unified JsOption Structure in TypeScript: It would be beneficial to have a consistent JsOption DTO structure on the js side. This would involve introducing ts, which could greatly enhance readability and error-checking. With source generators, we could automate much of this without requiring manual duplication of JsOption on both sides. This transition could be done gradually, component by component, for a smooth adoption.

@stsrki stsrki changed the title ButtonJsOptions.cs and ColorPickerJsOptions.cs Refactor: add strongly typed serialization objects Nov 8, 2024
@tesar-tech
Copy link
Collaborator Author

I have a question about internal vs public data types in options. @stsrki

Currently, VideoJSOptions is internal, which prevents it from being used in a public module method:

    public virtual async ValueTask Initialize(DotNetObjectReference<Video> dotNetObjectReference, ElementReference elementRef, string elementId, VideoInitializeJSOptions options)

One solution could be to make the JSModule methods internal as well. Is there any reason not to, or are people actively calling these methods?

Alternatively, we could make all *JSOptions classes public, but placing them in the main Blazorise namespace might not be ideal. Would moving them to a dedicated namespace be a better option?

Second question is about the interfaces for the JS modules, as I already asked in the PR description—do we really need them? For example, the JSVideoModule doesn’t have an interface and seems to work just fine.

@stsrki
Copy link
Collaborator

stsrki commented Nov 11, 2024

I have a question about internal vs public data types in options. @stsrki

Currently, VideoJSOptions is internal, which prevents it from being used in a public module method:

    public virtual async ValueTask Initialize(DotNetObjectReference<Video> dotNetObjectReference, ElementReference elementRef, string elementId, VideoInitializeJSOptions options)

One solution could be to make the JSModule methods internal as well. Is there any reason not to, or are people actively calling these methods?

Alternatively, we could make all *JSOptions classes public, but placing them in the main Blazorise namespace might not be ideal. Would moving them to a dedicated namespace be a better option?

Second question is about the interfaces for the JS modules, as I already asked in the PR description—do we really need them? For example, the JSVideoModule doesn’t have an interface and seems to work just fine.

It might be that, in this case, VideoJSOptions was made public by accident. Not sure. But generally, we expose it as public in case we, or our users, need to do unit testing on the JS modules. Which, in turn, requires that JS options also be made public.

@tesar-tech tesar-tech marked this pull request as ready for review November 11, 2024 18:56
Copy link
Collaborator

@stsrki stsrki left a comment

Choose a reason for hiding this comment

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

Overall looks good: But there are still few things:

  • all public classes and properties must have XML comments written. If you don't already a GH copilot is good with it. And you can also use chatgpt to generate them,
  • Please use VS instead of Rider if you can. I see a lot of formating broken. Or if Rider can use .editorconfig then configure it.

@stsrki
Copy link
Collaborator

stsrki commented Nov 12, 2024

  • Please use VS instead of Rider if you can. I see a lot of formating broken. Or if Rider can use .editorconfig then configure it.

PS. https://www.jetbrains.com/help/rider/Using_EditorConfig.html#roslyn

@stsrki stsrki merged commit 05dd9c2 into master Nov 13, 2024
2 checks passed
@stsrki stsrki deleted the dev/serialization-objects-for-jsinterop branch November 13, 2024 08:20
@github-actions github-actions bot locked and limited conversation to collaborators Nov 13, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make a serialization object for JS interops
3 participants