event-driven-io / emmett

Emmett - a Node.js library taking your event-driven applications back to the future!
https://event-driven-io.github.io/emmett/
195 stars 19 forks source link

Support for an async `decide` / `handle` function for `CommandHandler` #99

Closed alex-laycalvert closed 1 month ago

alex-laycalvert commented 3 months ago

IRT: https://discord.com/channels/1211601942984532018/1270107659755847772/1270107980100145203

I am utilizing the built-in CommandHandler and have some business logic that requires me to pull data from an external source that will have to be async. Basically, it's checking whether a person attempting to perform an action is within a certain group, that depends on my event sourced model's properties, but this group's users change frequently and would not make sense to store on the state directly besides a group ID.

So, in order to perform the logic, I need to do some async function calls, this should be enough to support it since the command handler this returns is already async, there shouldn't be a problem with the extra await and type defs for handle.

Let me know if this is not desired for specific reasons and I'll close it. Thank you for your help!

alex-laycalvert commented 3 months ago

I think I could get around this by passing in more data to the initialState function that comes from the external sources, but some of this data depends on the current state of the model and it could end up with a lot of extra properties for the initial state when a couple async calls depending on the specific command being passed would be simpler

oskardudycz commented 3 months ago

@alex-laycalvert my recommendation would be to do something like it's visible in this sample, so:

export const shoppingCartApi =
  (
    eventStore: EventStore,
    eventPublisher: EventsPublisher,
    getUnitPrice: (_productId: string) => Promise<number>,
    getCurrentTime: () => Date,
  ): WebApiSetup =>
  (router: Router) => {
    // Add Product Item
    router.post(
      '/clients/:clientId/shopping-carts/current/product-items',
      on(async (request: AddProductItemRequest) => {
        const shoppingCartId = getShoppingCartId(
          assertNotEmptyString(request.params.clientId),
        );
        const productId = assertNotEmptyString(request.body.productId);

        const command: AddProductItemToShoppingCart = {
          type: 'AddProductItemToShoppingCart',
          data: {
            shoppingCartId,
            productItem: {
              productId,
              quantity: assertPositiveNumber(request.body.quantity),
              unitPrice: await getUnitPrice(productId),
            },
          },
          metadata: { now: getCurrentTime() },
        };

        await handle(eventStore, shoppingCartId, (state) =>
          addProductItem(command, state),
        );

        return NoContent();
      }),
    );
);

We're passing the explicit getUnitPrice as an example of how to query an external system. I'm mapping that to the command.

State or initial state represents the stream's state, so data is inside the stream boundary. That's important, as for that data, stream is the source of truth. Data that is coming from "the outside world" typically should be passed as command.

My recommendation would be to use such a rule of thumb.

One of the options that I'd consider more is providing the customisable middleware to inject inside the command handler. That would be essentially a wrapper for the Command Handler decide function, and that could be made asynchronous.

I'm not totally against this change, but I'm concerned that many people will try to abuse that, thinking that running asynchronous stuff in the business logic is fine. Your use case is valid, as it's more of a precondition check (thus my suggestion about adding middleware), but typically it's not, as such async stuff can dim the boundaries.

@alex-laycalvert, thoughts?

oskardudycz commented 1 month ago

@alex-laycalvert Though I stand by my past recommendations, then after rethinking, I see that addition as useful in certain cases, and that it can be a decent starting point for a pipeline processing. I'm going to pull that in :+1: