EventFlow icon indicating copy to clipboard operation
EventFlow copied to clipboard

Query Processing IoC Refactor

Open WardenUnleashed opened this issue 4 years ago • 9 comments

I have been working on integrating SignalR with EventFlow and was able to get it working at a rudimentary level.

The integration required some pre-work to get everything working correctly and that work is some that I felt others could benefit from!

I'm hoping I can get some feedback / confirmation this is change that would be desirable. If so, I'll add the relevant tests to it!

Getting this in for v1 would be ideal since it has breaking changes to queries(namely the addition of a method that works similar to the one on the Command class)

Additionally, I noticed that the ISerializedCommandPublisher only returns the ISourceId while consuming the IExecutionResult. It made me wonder if instead it should return an IExecutionResult which in turn should have the ISourceId from the command appended to it. This way, you could always link a command to its results! What do you think?

WardenUnleashed avatar Dec 29 '20 02:12 WardenUnleashed

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

:white_check_mark: rasmus
:x: Andrew Johnson


Andrew Johnson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

CLAassistant avatar Dec 29 '20 02:12 CLAassistant

Hi @WardenUnleashed this looks interesting and it would make it easier to integrate queries where there's no generic type

rasmus avatar Jan 11 '21 13:01 rasmus

Added a refactor of the query processor's IoC. Since this would be a breaking query change any, I feel like rolling them together would be good!

WardenUnleashed avatar Jan 24 '21 04:01 WardenUnleashed

Hi @WardenUnleashed

Have a look at what I did in #825, it implements the method that you were missing on the IQueryProcessor, without any need to change existing queries and adding another method on the IQueryHandler.

I do believe that EventFlow should try to keep all the nasty stuff bundled within itself and try to keep the interfaces that developers need to implement as simple as possible. Thus adding an extra method IQueryHandler merely to make it easier to implement in EventFlow, isn't a good solution.

Last time I some tests on the ReflectionHelper.CompileMethodInvocation implementation, it was 400-600 times as efficient as doing a MethodInfo.InvokeMethod call, as it generates IL code and compiles it runtime, basically its a very efficient type cast.

Maybe it can be used to remove the task.GetType().GetTypeInfo().GetProperty("Result").GetValue(task) part from the PoC I suggested.

If the implementation from my small PR was added, then no breaking changes are needed (as far as I can see) and we could add this feature to the 0.x release.

rasmus avatar Jan 24 '21 08:01 rasmus

Nice! That seems like the better option for now! I'll refactor my branch over this week to get it in line!

WardenUnleashed avatar Jan 24 '21 19:01 WardenUnleashed

@rasmus Finally was able to get back around to this! Not so many changes required and looks like it could be added to v0! Also added unit tests!

WardenUnleashed avatar Apr 03 '21 21:04 WardenUnleashed

Nice, I'll have a look

On 3 Apr 2021, at 23.55, Andrew Johnson @.***> wrote:

 @rasmus Finally was able to get back around to this! Not so many changes required now! Also added unit tests!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

rasmus avatar Apr 03 '21 22:04 rasmus

@rasmus Just giving you another ping to see if this can go into develop-v1 soon :D

WardenUnleashed avatar May 05 '21 21:05 WardenUnleashed

@WardenUnleashed there seems to be a few tings wrong with the branch, spent a lot of time this evening trying to get it working on AppVeyor. I'll try to get it working asap

rasmus avatar May 06 '21 18:05 rasmus

Hello there!

We hope this message finds you well. We wanted to let you know that we have noticed that there has been no activity on this pull request for the past 90 days, which makes it a stale pull request.

As a result, we will be closing this pull request within the next seven days. If you still think this pull request is necessary or relevant, please feel free to update it or leave a comment within the next seven days.

Thank you for your contributions and understanding.

Best regards, EventFlow

github-actions[bot] avatar Apr 08 '23 13:04 github-actions[bot]

Hello there! I'm a bot and I wanted to let you know that your pull request has been closed due to inactivity after being marked as stale for seven days. If you believe this was done in error, or if you still plan to work on this pull request, please don't hesitate to reopen it and let us know. We're always happy to review and merge high-quality contributions. Thank you for your interest in our project! Best regards, EventFlow

github-actions[bot] avatar Apr 16 '23 09:04 github-actions[bot]