opossum icon indicating copy to clipboard operation
opossum copied to clipboard

Suggestion: Change `CircuitBreaker` type declaration

Open jeengbe opened this issue 1 year ago • 5 comments

I propose the following change to @types/opossum:

// From
declare class CircuitBreaker<TI extends unknown[] = unknown[], TR = unknown> extends EventEmitter { ... }

// To
declare class CircuitBreaker<T extends (...args: readonly never[]) => Promise<unknown>> extends EventEmitter { ... }

While this is the most breaking change, it makes a lot of work with the CircuitBreaker interface easier, for example:

export class CircuitBreakerThingRepository implements ThingRepository {
  private readonly circuitBreaker: CircuitBreaker<
    Parameters<ThingRepository['getThing']>,
    Awaited<ReturnType<ThingRepository['getThing']>>
  >;

  constructor(
    circuitBreakerFactory: CircuitBreakerFactory,
    delegate: ThingRepository,
  ) {
    this.circuitBreaker = circuitBreakerFactory.create(
      delegate.getThing.bind(delegate),
    );
  }

  getThing(arg: number): Promise<string> {
    return this.circuitBreaker.fire(arg);
  }
}

to

  private readonly circuitBreaker: CircuitBreaker<ThingRepository['getThing']>;

Practically, the type annotation is not required here (TS is able to infer it given the constructor assignment). Code such as:

export interface CircuitBreakerFactory {
  create<T extends (...args: readonly never[]) => unknown>(
    implementation: T,
  ): CircuitBreaker<Parameters<T>, Awaited<ReturnType<T>>>;
}

is also simpler written like:

export interface CircuitBreakerFactory {
  create<T extends (...args: readonly never[]) => Promise<unknown>>(
    implementation: T,
  ): CircuitBreaker<T>;
}

I am willing to submit a PR.

jeengbe avatar Feb 13 '24 20:02 jeengbe

submitting a PR would be helpful

lholmquist avatar Feb 19 '24 19:02 lholmquist

See the above PR.

jeengbe avatar Feb 21 '24 19:02 jeengbe

PR is ready to be merged, what's the status for this?

jeengbe avatar Mar 20 '24 08:03 jeengbe

The DefinetlyTyped repo is a community created repo and isn't associated with this repo, so you would need to ask those folks when it will be merged

lholmquist avatar Mar 21 '24 16:03 lholmquist

This issue is stale because it has been open 30 days with no activity.

github-actions[bot] avatar Apr 21 '24 00:04 github-actions[bot]