Feedback regarding subscribe/execute API

[n1ru4l]

Execute

https://github.com/dotansimha/envelop/blob/b6573398738208f38a721c82d861d0fee14d1679/packages/types/src/index.ts#L62-L68

I think the API right now is not suited for streams (defer, stream, live) and should be adjusted accordingly.

onExecuteDone

In case of stream/defer/live operations we could go multiple routes:

Possible adjustment options for onExecutionDone

1. Invoke onExecutionDone with the last/final result

Edit: Now that I reflected some more on it I am no longer sure whether this makes the most sense. For a Promise value, we know when something is the last value. With defer/stream we can kind of detect the last result via result.hasNext, but that might again be digging too much into spec details and allows less experimentation.

With my current @live implementation there is no hasNext in the execution result. When should onExecutionDone be invoked? When the client notifies he is no longer interested in results for this operation? And then we do not really have the last execution result unless we start to always store the latest execution result 🤔.

2. Invoke onExecutionDone with an array of all results

Potential memory leak for @live operations, and huge @stream lists.
If anyone needs all the results, it can be done by using onExecutionValue (or a similar proposed alternative API) for collecting all those values.

3. Invoke onExecutionDone with the merged results

envelop must be aware of how results can be merged and would make it less flexible for experiments such as @live.
If anyone needs the merged result, it can be done by using onExecutionValue (or a similar proposed alternative API) for collecting/merging all those values.

4. Invoke onExecutionDone with the async iterable

The more bare-bones solution; gives a lot of control back to the user. But the wording onExecutionDone would kind of be misleading as the execution is done after the async iterable returns.

Using this kind of API implies that we don't wanna mess with AsyncIterableIterators and give them to the user. So I think this is not the way we should choose.

5. Break stuff more down and add some kind of onExecutionStream API

Have different handlers for Promise<ExecutionResult> | ExecutionResult and AsyncIterableIterator<ExecutionResult>.

onExecutionDone should be invoked for execute calls that result in Promise<ExecutionResult> | ExecutionResult ?
onExecutionStream should be invoked for execute calls that result in AsynIterableIterator<ExecutionResult> ? Further handlers can be returned from this call for hooking into onNext and onReturn.

type OnExecutionStreamHandlerHookResult = {
  /* Alternative name: onNext? */
  onValue?: (
    executionResult: ExecutionResult,
    setExecutionResult: (
      result: ExecutionResult
    ) => void
  ) => void;
  onReturn?: () => void;
};

type OnExecutionStreamHandler = (options: {
  executionResult: AsyncIterableIterator<ExecutionResult>;
  setExecutionResult: (
    result: AsyncIterableIterator<ExecutionResult> | ExecutionResult
  ) => void;
}) => OnExecutionStreamHandlerHookResult | void;

export type OnExecuteHookResult<ContextType = DefaultContext> = {
  /* Invoked if `execute` returns Promise<ExecutionResult> | ExecutionResult */
  onExecuteDone?: (options: {
    result: ExecutionResult;
    setResult: (newResult: ExecutionResult) => void;
  }) => void;
  /* Invoked if `execute` returns AsyncIterableIterator<ExecutionResult> */
  onExecutionStream?: OnExecutionStreamHandler;
};

Conclusion

Option 5 seems to make the most sense.

onExecutionValue

NOTE: This might be redundant with option 5. from the onExecutionDone section.

This should be invoked before onExecuteDone and furthermore be invoked for every value published/returned by the execute the call.

This means for the resolved Promise value (if it is not an AsyncIterable) or each value published by the AsyncIterable.

It would allow modification and manipulation of all the values.

Potential use-cases

Live query: Diff last and next execution result and send JSON patch to the clients instead.

logging

API Draft

type OnExecutionValueHandler = (options: {
  executionResult: ExecutionResult;
  setExecutionResult: (result: ExecutionResult) => void;
}) => void;

export type OnExecuteHookResult<ContextType = DefaultContext> = {
  onExecuteDone?: (options: {
    result: ExecutionResult;
    setResult: (newResult: ExecutionResult) => void;
  }) => void;
  onResolverCalled?: OnResolverCalledHooks<ContextType>;
  onExecutionValue?: OnExecutionValueHandler;
};

Subscribe

https://github.com/dotansimha/envelop/blob/b6573398738208f38a721c82d861d0fee14d1679/packages/types/src/index.ts#L69-L73

Potentially new APIs

Note: All of these can also be done via function composition, in user-land, however, the hooks might be more intuitive and easier to use.

onSubscribe(Success)

Edit: The same API for execute as proposed in solution 5. (differentiate between the stream and single values) above might make more sense.

Invoked when the subscription is set up successfully (subscribe returns/resolves to an AsyncIterableIterator<ExecutionResult>).

This is kind of redundant with onSubscribeResult, but more user-friendly if you wanna do one specific thing when an AsyncIterable subscription has been created.

type OnSubscribeSuccessHandler = (options: {
  asyncIterable: AsyncIterableIterator<ExecutionResult>;
  setAsyncIterable: (
    asyncIterable: AsyncIterableIterator<ExecutionResult>
  ) => void;
}) => void;

export type OnSubscribeHookResult<ContextType = DefaultContext> = {
  onSubscribeResult?: (options: {
    result: AsyncIterableIterator<ExecutionResult> | ExecutionResult;
    setResult: (
      newResult: AsyncIterableIterator<ExecutionResult> | ExecutionResult
    ) => void;
  }) => void;
  onResolverCalled?: OnResolverCalledHooks<ContextType>;
  onSubscribeSuccess?: OnSubscribeSuccessHandler;
};

onSubscribeError

Edit: The same API for execute as proposed in solution 5. (differentiate between the stream and single values) above might make more sense.

Invoked when the subscription is set up successfully (subscribe returns/resolves to an ExecutionResult).

This is kind of redundant with onSubscribeResult, but more user-friendly if you wanna do one specific thing when an AsyncIterable subscription has not been created.

Potential use-cases might be resource cleanup e.g. disposing of a connection that was created for the context or logging.

type OnSubscribeErrorHandler = (options: {
  executionResult: ExecutionResult;
  /** Recover from error and setup subscription via custom asyncIterable, or modify execution result error **/
  setExecutionResult: (
    result: ExecutionResult | AsyncIterableIterator<ExecutionResult>
  ) => void;
}) => void;

export type OnSubscribeHookResult<ContextType = DefaultContext> = {
  onSubscribeResult?: (options: {
    result: AsyncIterableIterator<ExecutionResult> | ExecutionResult;
    setResult: (
      newResult: AsyncIterableIterator<ExecutionResult> | ExecutionResult
    ) => void;
  }) => void;
  onResolverCalled?: OnResolverCalledHooks<ContextType>;
  onSubscribeError?: OnSubscribeErrorHandler;
};

onValuePublished

Edit: The same API for execute as proposed in solution 5. (differentiate between the stream and single values) above might make more sense.

Mapping/modifying the ExecutionResult values published by the AsyncIterable returned by subscribe.

Less cumbersome than having to do an isAsyncIterable check and having to figure out how to map an async iterable compared to doing the same thing within onSubscribeResult. 🤔
Also great for logging.

type OnValuePublishedHandler = (options: {
  executionResult: ExecutionResult;
  setExecutionResult: (result: ExecutionResult) => void
}

export type OnSubscribeHookResult<ContextType = DefaultContext> = {
  onSubscribeResult?: (options: {
    result: AsyncIterableIterator<ExecutionResult> | ExecutionResult;
    setResult: (newResult: AsyncIterableIterator<ExecutionResult> | ExecutionResult) => void;
  }) => void;
  onResolverCalled?: OnResolverCalledHooks<ContextType>;
  onValuePublished?: OnValuePublishedHandler;
};

onSubscribeEnd

Edit: The same API for execute as proposed in solution 5. (differentiate between the stream and single values) above might make more sense.

type OnSubscribeEndHandler = () => void;

export type OnSubscribeHookResult<ContextType = DefaultContext> = {
  onSubscribeResult?: (options: {
    result: AsyncIterableIterator<ExecutionResult> | ExecutionResult;
    setResult: (
      newResult: AsyncIterableIterator<ExecutionResult> | ExecutionResult
    ) => void;
  }) => void;
  onResolverCalled?: OnResolverCalledHooks<ContextType>;
  onSubscribeEnd?: OnSubscribeEndHandler;
};

Potential use-cases might be resource cleanup e.g. disposing of a connection that was created for the context or logging.

[dotansimha]

Thank you @n1ru4l . This is amazing. Here are a few thoughts regarding this.

Regarding execute:

For @live / @stream / @defer, we definitely need a more sophisticated mechanism. I guess we can go with calling onExecuteDone when the first result is being triggered, and then consider an additional API for triggering more values that are being emitted later. What do you thing?

onExecuteDone({ result, isStream }) => on first value for stream, on done when non-stream, with a boolean to indicate if we need to expect more values. This should be fine I guess, mainly because the first result is "complete" so it matches the regular execution (promise-based).
onStreamValue => for every value triggered from a stream, after the first result. This might be partial, so we I think we need an API for that. I think this is also similar to option #5 you suggested,

Regarding subscribe:
Totally agree with this approach. I liked option #5 that you suggested, and exposing simple functions on top of subscriptions seems like a good approach for me. It also means that we wrap the AsyncIterable, right? Because if we wish to allow users to modify values, we must wrap it to allow flexibility and low-level API.

[n1ru4l]

@dotansimha see aea90c4 for a bit messy poc by returning a new object from onExecuteDone and wrapping the iterables

[dotansimha]

#428