Skip to content
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

add clock #95

Merged
merged 2 commits into from
Dec 4, 2021
Merged

add clock #95

merged 2 commits into from
Dec 4, 2021

Conversation

DavidBadura
Copy link
Member

@DavidBadura DavidBadura commented Dec 1, 2021

I think it would be a good idea to add a clock implementation to this library. We can use it to write our tests better and the users can also manipulate the time for their tests. This is the missing part for the "testing" documentation.

@DavidBadura DavidBadura added this to the 1.0.0 milestone Dec 1, 2021
@DanielBadura
Copy link
Member

I dont think we should do this. The user has already the possibility to replace the method createRecordDate with are "more suitable" one if desired. And manipulating DateTime is always tricky and really painful if done wrong. I think with #58 we already did the job. We dont really need this for our testing usecase, do we?

@DavidBadura
Copy link
Member Author

Yes, you can override it, but it is not good for the dx if the user need to create an own "mock" or "class" to manage clock features. Other event sourcing libraries do it too: https://eventsauce.io/docs/utilities/clock/

@DanielBadura
Copy link
Member

DanielBadura commented Dec 1, 2021

Hm, i can see the need for it generally but the solution used by eventsauce is somewhat cleaner from my PoV since it's a different class used for mocking and normal usage. With our solution here it could happen that in production the clock is frozen.

But i cannot provide another solution to this tbh since we cannot inject different classes here in this situation. That's why I'm concered about the consequeces for some dx could be a fatal failure for production. I know, with the eventsauce approach this could also happen, but it seems for me that this is more unlikely.

Maybe a path between would be suitable? Just add the clock class but not directly use it in the AggregateChanged? So the user can add it fairly easily?

@DanielBadura DanielBadura merged commit d7e3979 into master Dec 4, 2021
@DanielBadura DanielBadura deleted the add-clock branch December 4, 2021 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants