Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set type of transferList to workerThreads.TransferListItem #152

Merged
merged 2 commits into from
May 31, 2024

Conversation

kylejeske
Copy link
Contributor

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
Copy link

coveralls commented May 28, 2024

Pull Request Test Coverage Report for Build 9289406492

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 49.907%

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

💛 - Coveralls

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kylejeske
Copy link
Contributor Author

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:

emit(eventName: 'message', message: any, transferList?: Transferable[]): boolean

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

@kylejeske kylejeske changed the title Ensure DOM lib is available for type Transferable Set type of transferList to workerThreads.TransferListItem May 29, 2024
@kylejeske kylejeske requested a review from mcollina May 29, 2024 16:18
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mcollina mcollina merged commit de08119 into pinojs:main May 31, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants