-
-
Notifications
You must be signed in to change notification settings - Fork 534
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
Conversation
I have a question about internal vs public data types in options. @stsrki Currently, public virtual async ValueTask Initialize(DotNetObjectReference<Video> dotNetObjectReference, ElementReference elementRef, string elementId, VideoInitializeJSOptions options) One solution could be to make the Alternatively, we could make all 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 |
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. |
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.
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.
Source/Extensions/Blazorise.SignaturePad/SignaturePadJSOptions.cs
Outdated
Show resolved
Hide resolved
PS. https://www.jetbrains.com/help/rider/Using_EditorConfig.html#roslyn |
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
object
toTheComponentJsOptions
inJsModule
and its interface, as well as when calling methods in the module (typically withinTheComponent.razor.cs
).record
Types: Given that we're targeting .NET 6, therequired
keyword isn’t available. Usingrecord
types is a suitable alternative, as it allows for clean initialization without needingSomeProperty = 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.JsOptions
files are organized within aJsOptions
folder. I followed the codebase conventions, placing everything within theBlazorise
namespace.Questions
JsModules
: I’m unclear on the purpose of having interfaces forJsModules
, 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
JsOption
Structure in TypeScript: It would be beneficial to have a consistentJsOption
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 ofJsOption
on both sides. This transition could be done gradually, component by component, for a smooth adoption.