Feature/aggregate di #52
Conversation
|
I need to prioritise the next release's breaking changes and there's a bug we need to investigate too. This can go to a minor release 3.x.0 if needed, but having said that, are there any issues with this from your point of view, or do we just need code review? |
|
Forgot there is better way to handle |
|
Ok, then let's bring it into 3.0.0 as it's a nice feature worthy of the major release |
# Conflicts: # source/server/infrastructure/router.coffee
| } | ||
|
|
||
| eventMap: -> { | ||
| 'CustomerApp.CustomerCreated': (event) -> @name = event.customerName | ||
| 'CustomerApp.CustomerCreated': (event) -> | ||
| @myEventDependency() |
There was a problem hiding this comment.
Is there a use-case for this within a state-change event handler?
There was a problem hiding this comment.
I think none, misunderstood the concept:
Aggregate:
Fired command on new Aggregate -> Router::_setupInitializingMessage -> new instance and injection
Fired command on existing Aggregate -> Router::_genericCommandHandler -> find instance and injection
events on both paths still have access to dependencies.
Process:
Fired command/routed event on new Process -> Router::_setupInitializingMessage -> new instance and injection
Routed event on new Process -> Router::_genericEventHandler -> find instance and injection (so thats why this only relates to EXTERNAL EVENT HANDLERS)
Is that right?
There was a problem hiding this comment.
This is looking good
events on both paths still have access to dependencies.
This is expected, it's up to the implementor to know where not to use deps :-). We will need to have clear instructions for:
• aggregate state changing event handlers should never use an external dependency
• provide use-cases for when to use an external dependency prior to state-change
At a later time it would be ideal to abstract the internal state-change event mapping, as we could then just provide a mapping without the option of accessing a dependency.
|
See: meteor-space/messaging#28 I think this in combination with CommandBus/EventBus middleware will be the best approach for command validation and configuration-based business rules. Let's try to wrap the feature here so it's in the 3.0.0 release. |
|
@DominikGuzei I've just assigned this to you for review. Looks good to me |
| 'tests/infrastructure/handling-domain-errors.tests.js' | ||
| 'tests/infrastructure/handling-domain-errors.tests.js', | ||
| // INTEGRATION | ||
| 'tests/integration/event-sourceable_dependency_injection.js' |
There was a problem hiding this comment.
please let's just use dashes in filenames to stay consistent
|
Just punted this non-breaking feature to the next feature release as it's now the only blocker for 3.0.0 |
Direct talking with Injector dependency on Router rather then using _injectDependencies wrapper
# Conflicts: # tests/test_application.coffee
|
We need changelog updated and version number before the release |
|
Ah sorry for that, im deep in another project and problem solving 😉 should not merge in that case. |
|
It is fine. We will cover that in release branch. |
@meteor-space/core In future let's try to ensure any new features are added to the changelog under the heading "next". Version numbers should remain only a concern of the release, managed in a separate branch. |
Yes, this is a very good practice introduced by Rhys which makes releases easy. And author of the change probably is the best person to write what is changed rather then reviews. I would also suggest that we add snippets of examples how new features can be used. We can move that to proper documentation and link to it from the changelog eventually. Shall we restore this branch? |
|
No a merge is a merge, if anything a new chore branch |
|
OK, than we should delete this branch and create a new one. |
This is one possible approach for creating the documentation (for new features that must touch on some existing features and therefore have even more value) while generating documentation from comments is not there. |
|
@meteor-space/core Shall we consider releasing this? |
|
Imo we should have examples pointing out good and bad practices here - since someone can assume full freedom while using DI and its not the case here. |
|
I started using this today. I have real world examples. Maybe we should join or experiences @qejk and continue working on this. This is really good and worthy contribution to ES aggregates/processes. We should also address current implementation of aggregate states. And messaging middleware. |
No description provided.