Azure / azure-functions-durable-js

JavaScript library for using the Durable Functions bindings
https://www.npmjs.com/package/durable-functions
MIT License
128 stars 47 forks source link

Switch APIs from using discrete optional arguments to using an options argument where it makes sense #441

Closed hossam-nasr closed 1 year ago

hossam-nasr commented 1 year ago

Based on discussion here. This issue is a generalization of #415 and #435, to switch all of our APIs from using optional discrete arguments to accepting an options object argument, where applicable and where it makes sense.

hossam-nasr commented 1 year ago

APIs affected/proposed:

In DurableOrchestrationClient:

Before Proposed change
```TS getStatus( instanceId: string, showHistory?: boolean, showHistoryOutput?: boolean, showInput?: boolean ): Promise ``` ```TS getStatus( instanceId: string, options?: GetStatusOptions ): Promise ```
```TS getStatusBy( createdTimeFrom: Date | undefined, createdTimeTo: Date | undefined, runtimeStatus: OrchestrationRuntimeStatus[] ): Promise ``` ```TS getStatusBy( options: SelectionOptions ): Promise ```
```TS purgeInstanceHistoryBy( createdTimeFrom: Date, createdTimeTo?: Date, runtimeStatus?: OrchestrationRuntimeStatus[] ): Promise ``` ```TS purgeInstanceHistoryBy( options: SelectionOptions ): Promise ```
```TS raiseEvent( instanceId: string, eventName: string, eventData: unknown, taskHubName?: string, connectionName?: string ): Promise ``` ```TS raiseEvent(options: RaiseEventOptions): Promise ```
```TS readEntityState( entityId: EntityId, taskHubName?: string, connectionName?: string ): Promise> ``` ```TS readEntityState( entityId: EntityId, options?: TaskHubOptions ): Promise> ```
```TS rewind( instanceId: string, reason: string, taskHubName?: string, connectionName?: string ): Promise` ``` ```TS rewind( instanceId: string, reason: string, options?: TaskHubOptions ): Promise ```
```TS signalEntity( entityId: EntityId, operationName?: string, operationContent?: unknown, taskHubName?: string, connectionName?: string ): Promise ``` ```TS signalEntity( entityId: EntityId, options?: SignalEntityOptions ): Promise ```
```TS startNew( orchestratorFunctionName: string, instanceId?: string, input?: unknown ): Promise ``` ```TS // this change is already merged startNew( orchestratorFunctionName: string, options?: StartNewOptions ): Promise ```

In DurableOrchestrationContext:

Before Proposed Change
```TS callSubOrchestrator( name: string, input?: unknown, instanceId?: string ): Task ``` ```TS // This is effectively equivalent // to the startNew() change callSubOrchestrator( name: string, options?: StartNewOptions ): Task ```
```TS callSubOrchestratorWithRetry( name: string, retryOptions: RetryOptions, input?: unknown, instanceId?: string ): Task ``` ```TS callSubOrchestratorWithRetry( name: string, retryOptions: RetryOptions, options?: CallSubOrchestratorOptions ): Task ```
```TS callHttp( method: string, uri: string, content?: string | object, headers?: { [key: string]: string }, tokenSource?: TokenSource, asynchronousPatternEnabled = true ): Task ``` ```TS // This change is already merged callHttp(options: CallHttpOptions): Task ```
davidmrdavid commented 1 year ago

Thanks @hossam-nasr. I like the idea of modernizing these through an options object, but given the size of the list (over 10 APIs), I would have to request that any change to these should be backwards compatible.

Have we already made callHttp and startNew backwards compatible with the old style?

ejizba commented 1 year ago

Unfortunately we have to do overloads to be backwards compatible, and overloads pretty negatively impact intellisense and build errors. I would prefer we choose between a clean yes or no on this change

davidmrdavid commented 1 year ago

Understandable. At this time, I do not feel comfortable taking the entirety of this list as breaking changes. Let me reach out internally for some ideas on how to move forward.

hossam-nasr commented 1 year ago

Closed by #482