pinojs / thread-stream

A streaming way to send data to a Node.js Worker Thread
MIT License
229 stars 23 forks source link

Set type of transferList to workerThreads.TransferListItem #152

Closed kylejeske closed 4 months ago

kylejeske commented 4 months ago

In certain environments, index.d.ts will generate a typing error when thread-stream is included a dependency of pino, and TS config (skipLibCheck) is not set to true.

Before:

$ [other-package] > npx tsc

TS2304: Cannot find name 'Transferable'.

89   emit(eventName: 'message', message: any, transferList?: Transferable[]): boolean
                                                             ~~~~~~~~~~~~
Found 1 error in ../../../../thread-stream/index.d.ts:90

After:

$ [other-package] > npx tsc

Adding the reference makes the compiler aware of the reference to the library, even if the package consuming it, does not have the necessary types included.

coveralls commented 4 months ago

Pull Request Test Coverage Report for Build 9289406492

Details


Totals Coverage Status
Change from base Build 9212428238: 0.0%
Covered Lines: 201
Relevant Lines: 367

💛 - Coveralls
kylejeske commented 4 months ago

Unfortunately that fix is not correct, because this lib does not use the DOM. The correct way is to get https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d8da2b742fb2d05ad786d8afb42d1032c0fa2eff/types/node/worker_threads.d.ts#L91.

Thanks for providing this ... it seemed strange to me to need a DOM lib for a backend implementation.

Based on the information you just gave, should the implementation use TransferListItem[] instead of the Transferable[] type then?

index.d.ts:

  /**
   * Post a message to the Worker with specified data and an optional list of transferable objects.
   *
   * @param eventName the name of the event, specifically 'message'.
   * @param message message data to be sent to the Worker.
   * @param transferList an optional list of transferable objects to be transferred to the Worker context.
   * @returns {boolean} true if the event had listeners, false otherwise.
   */
  emit(eventName: 'message', message: any, transferList?: workerThreads.TransferListItem[]): boolean

current implementation reads:

index.d.ts: ref: https://github.com/pinojs/thread-stream/blob/88d852406d3700ef452a916e8df36546dee87c8c/index.d.ts#L89

the only type I could find for "Transferable" was within the DOM library.

type Transferable = OffscreenCanvas | ImageBitmap | MessagePort | ReadableStream<any> | WritableStream<any> | TransformStream<...> | VideoFrame | ArrayBuffer

also noticed these two types have some overlap ...

type Transferable = OffscreenCanvas | ImageBitmap | MessagePort | ReadableStream | WritableStream | TransformStream | VideoFrame | ArrayBuffer;
type TransferListItem = ArrayBuffer | MessagePort | FileHandle | X509Certificate | Blob;

ref: Type: Transferable https://github.com/microsoft/TypeScript/blob/fa58c615a43d0c2c9ed60ef4ff4783da91c0ce32/src/lib/dom.generated.d.ts#L28393

ref: Type: TransferListItem https://github.com/DefinitelyTyped/DefinitelyTyped/blob/d8da2b742fb2d05ad786d8afb42d1032c0fa2eff/types/node/worker_threads.d.ts#L91