search

pinojs / sonic-boom

Extremely fast utf8 only stream implementation
MIT License
261 stars 41 forks source link

Confusion with dest param in SonicBoomOpts type #116

Closed rossanthony closed 2 years ago

rossanthony commented 2 years ago

The type SonicBoomOpts defines dest as a string: https://github.com/pinojs/sonic-boom/blob/master/types/index.d.ts#L11, however it seems like you can also pass a number to it. As can be seen here within the pino-pretty/ library: https://github.com/pinojs/pino-pretty/blob/master/index.js#L193.

Should the type be updated to string|Number to allow for passing 1 to indicate stdout and a string if passing a path to a file?

I noticed that it's also possible to do this: SonicBoom({ fd: process.stdout.fd }) how does this differ from SonicBoom({ dest: 1 })? Or do both achieve exactly that same in piping the stream to stdout?

mcollina commented 2 years ago

I think the type should be updated.

cc @kibertoad

rossanthony commented 2 years ago

I can open a PR to correct the type, so it should be string|Number?

The readme for SonicBoom should also change to reflect this, specifically here, how about changing this:

  • fd: a file descriptor, something that is returned by fs.open or fs.openSync.

to:

  • fd: a file descriptor, something that is returned by fs.open or fs.openSync, or a number (1: stdout, 2: stderr).
mcollina commented 2 years ago

Or do both achieve exactly that same in piping the stream to stdout?

They both achieve the exact same thing.