Closed petertorocsik closed 7 months ago
This will be my first pull request of my life. I hope you find everything okay...
Hello. Thank you for this, warms my heart, this is only the second (Ithink) pull request I've got on this project.
I never got around to doing complex types because in my mind the expression serializer should be deprecated anyway. But here we are! And this is probably useful even if I ditch the expressions.
In any case, at a first glance, good work!
There are some things that are a miss,
The complextypes property should not be of type object (did you mean Type[] ?)
The workeroptions Merge method should probably do an array merge? Rather than just replacing any option already present
The javascript options are doing something with the options serializer that I don't quite understand (is this a json inside a json thing?)
Some kind of test/demo/usecase would be nice (I mostly do manual testing by adding a page to the demo project, I've been meaning to automate it with selenium...)
Hello, Thank you the praise and remarks. Congratulate for the project! I really need it to my dissertation.
The complextypes property should not be of type object (did you mean Type[] ?)
I chose the object type because the serialized version of CustomKnownTypes property can be in the same object and don't need to create 2 objects with the same name but different types in the same class or the serialized object doesn't need to be separate from WorkerInitOptions because I think it belongs there.
In that case I could use Type[] if the serialized customKnownTypes would be a separated parameter in WorkerProxy InitAsync and BlazorWorker.initWorker methods.
In the knowledge of this information let the property is Type[] rather?
The javascript options are doing something with the options serializer that I don't quite understand (is this a json inside a json thing?)
I use JSON parse method in initConf constant for customKnownTypes because in initConf customKnownTypes is given in quotation marks and the parse method called for initConf in BlazorWorker.workerDef indicates an error.
Did you mean this part?
Some kind of test/demo/usecase would be nice (I mostly do manual testing by adding a page to the demo project, I've been meaning to automate it with selenium...)
All right! I thought I would add complex types to the existing tests in the demo project so I don’t have to create redundantly all tests with complex types. I would only change the Five method with a custom type parameter. The custom class would have a method that would generate the value 5 and would inherit a base class and in this case this base class should also be given in the CustomKnownTypes property.
Would it be a good idea, or should I do separate test/tests?
the serialized version of CustomKnownTypes property can be in the same object
Sounds a bit confusing. However I was probably wrong about Type[] it should more likely be named CustomKnownTypeNames and be a string[] to be consistent with the other properties and avoid nested jsons and such.
add complex types to the existing tests in the demo project so I don’t have to create redundantly all tests with complex types.
A middle ground could be to add some logic to an existing test, to be able to test both with and without complex tests, without duplicating too much code Adding complex types to all test may take away from runnable simple examples.
In any case, i think the CustomKnownTypes signature is the only really important issue here as it's part of the public api and adds complexity to the serialization code. I'm hoping to get to try this out in the coming days!
I've tried the string option before, but I'm confused that it should have been serialized in InitOptions class, but the MessageSerializer interface isn't yet available in this class. I didn't think a good idea to calling the SerializeLinq's DefaultMessagSerializer or NewtonSoft's JsonConverter, because the BlazorWorker has the own serialize solution.
Then I just realized it wasn't necessary to serialize, because it's enough to just put the AssemblyQualifiedName values of the types into the string[] array, so it was a good idea to use this, thanks! It is possible to use the GetType method of the Type class to create the type from AssemblyQualifiedName property. Overall, the solution has become simpler.
To create the tests, I would now have an idea that I would create parameters for SharedPages components and from a new BlazorWorker.ComplexTypeTests named project would call them by specifying the parameters...
So I finished the development with the string[] type. Should I commit now without the test, or with the test altogether if it'll be ready?
If I do a commit now, will it automatically belong to this pull request as a second commit? I haven't done this yet...
So I finished the development with the string[] type. Should I commit now without the test, or with the test altogether if it'll be ready?
If I do a commit now, will it automatically belong to this pull request as a second commit? I haven't done this yet...
Just commit and push and the pull request will be updated, it's based on the head of your branch. So you can do it in any order you want
Thanks!
The new commit with push are ready. I'll create the tests and I'll apply when they done.
FIxed by #98
For credit, this fork has been merged through another pr (#103)
I created a complex type registration feature that can be specified in WorkerInitOptions Action with the AddCustomKnownTypes extension method when setting up a worker.