nodeshift / opossum

Node.js circuit breaker - fails fast ⚡️
https://nodeshift.dev/opossum/
Apache License 2.0
1.28k stars 106 forks source link

Suggestion: Change `CircuitBreaker` type declaration #846

Open jeengbe opened 7 months ago

jeengbe commented 7 months ago

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.

lholmquist commented 7 months ago

submitting a PR would be helpful

jeengbe commented 6 months ago

See the above PR.

jeengbe commented 6 months ago

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

lholmquist commented 5 months ago

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

github-actions[bot] commented 4 months ago

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