Event Type Mapping improvements #190
Conversation
| @@ -26,7 +26,7 @@ static EventStoreDBSerializer() | |||
| public static object? Deserialize(this ResolvedEvent resolvedEvent) | |||
| { | |||
| // get type | |||
| var eventType = EventTypeMapper.ToType(resolvedEvent.Event.EventType); | |||
| var eventType = EventTypeMapper.Instance.ToType(resolvedEvent.Event.EventType); | |||
There was a problem hiding this comment.
Hm I find it a bit disturbing that this has state and has not gone to the singleton pattern too. I'd pick either static or stateful and wired through DI throughout in order to allow one to set up multiple event mapping policies with different json converters etc. (The logical conclusion of this is of course that the mapping process should be managed at the category level, not a global level)
There was a problem hiding this comment.
See, I changed that from static class because of your comment :P Tbt, I don't want to push the serializer as a dependency, I don't want it to be replaceable, but the essential part of the event store. If I put it as a parameter, then I'd push it also forward everywhere. This is really invasive in the parts where I'm musing more functional composition and redundant. I'm happy to revise the last changes.
There was a problem hiding this comment.
Not sure what the exact comment was, but the bottom line is that if it's stateful, then you need to decide
- if there can be multiple instances. If that's a nice to have that some people will want, baking in Singleton pattern sends the wrong message, whereas
staticis clear enough about the current impl not being generalised in that direction yet - whether state can be written concurrently
| }; | ||
| } | ||
|
|
||
| public static ShoppingCart Default() => |
There was a problem hiding this comment.
never a good pattern. should not be in Domain. Fine if you want it as a helper in tests, but put it there then
|
|
||
| using static ShoppingCartEvent; | ||
|
|
||
| public abstract class StronglyTypedValue<T>: IEquatable<StronglyTypedValue<T>> where T : notnull |
There was a problem hiding this comment.
interesting related things https://github.com/jet/equinox/blob/ebca2357c12f5ef46bbbba6e417d153272798285/samples/Store/Domain/Infrastructure.fs#L8
(I tend not to get into using these - I just use FSharp.UMX instead of all this...
There was a problem hiding this comment.
I'll just reference it to https://event-driven.io/en/using_strongly_typed_ids_with_marten/. It's more about showing how this can be done, but I also wouldn't suggest doing that. In general, I think that strongly typed ids is quite often overkill :P
There was a problem hiding this comment.
👍 I guess the main thing is to demonstrate it once, and ideally not mixed with something else (do as I say, not as I do :D)
|
|
||
| protected StronglyTypedValue(T value) => Value = value; | ||
|
|
||
| public override string ToString() => Value.ToString()!; |
There was a problem hiding this comment.
that renders Guids with dashes etc, which can end up very messy - might be worth considering how to model where you want e.g. cart id not to have embedded dashes
There was a problem hiding this comment.
Agreed, still, as this is an illustration then, I'm not sure if it includes the specific format, then the reader might be wondering why I did that. Thoughts?
There was a problem hiding this comment.
For me the problem is that leaning on the default ToString of a type when you're trying to represent something as typesafe is a hole - I'd tend to either have a base StringId class with explicit translations, or leave abstract methods etc. (see the other types I linked for ways to remove that ambiguity/implicitness).
You can still do some CreateDefault helper that plugs in the ToString explicitly afterwards
I guess if you can take Guids out of the picture entirely, then the issue of how to render them is off the table too...
| productItems | ||
| .Union(new[] { new KeyValuePair<ProductId, Quantity>(productItem.ProductId, productItem.Quantity) }) | ||
| .GroupBy(ks => ks.Key) | ||
| .ToDictionary(ks => ks.Key, ps => Quantity.Parse((uint)ps.Sum(x => x.Value.Value))); |
There was a problem hiding this comment.
would be much clearer to have an AddOrUpdate helper a la ConcurrentDictionary and then have 2 simple bits of code that either add to current value or insert the value - thats why its a dictionary isnt it?
There was a problem hiding this comment.
That makes sense, although then I'd need to explain it in the article 😅
There was a problem hiding this comment.
I think they'd survive - you want to tease them into looking at the code on the github!
| ? new KeyValuePair<ProductId, Quantity>(p.Key, | ||
| Quantity.Parse(p.Value.Value - productItem.Quantity.Value)) | ||
| : p) | ||
| .Where(p => p.Value > Quantity.Parse(0)) |
There was a problem hiding this comment.
this select/where should be a TryGetValue with a default
oskardudycz
force-pushed
the
event_type_mapping
branch
from
35386be
to
492c8e7
Compare
Made slight improvements to EventTypeMapper, allowing it to be injected by DI