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

add transports constructor option #1111

Merged
merged 10 commits into from
Sep 8, 2021
Merged

add transports constructor option #1111

merged 10 commits into from
Sep 8, 2021

Conversation

Eomm
Copy link
Contributor

@Eomm Eomm commented Aug 31, 2021

Implements #1105

Opening as a draft to ask what do you prefer if:

  • users add unnecessary options like a destination: should pino throw an error?
  const instance = pino({
    name: 'logger',
    transport: {
      target: '#pino/file',
      options: { destination }
    }
  }, '/log/path')
  • are you ok with the new transports field name to manage both pino.transport() input? ( {target} or {targets:[]} )

Then I'm going to add more tests and doc as well

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.

Good work! I think it should throw if stream is otherwise defined.

lib/tools.js Outdated Show resolved Hide resolved
@Eomm Eomm marked this pull request as ready for review September 1, 2021 07:57
test/transport.test.js Outdated Show resolved Hide resolved
test/transport.test.js Outdated Show resolved Hide resolved
docs/api.md Show resolved Hide resolved
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

docs/api.md Outdated Show resolved Hide resolved
lib/tools.js Outdated Show resolved Hide resolved
docs/api.md Outdated Show resolved Hide resolved
The `transport` option is a shortcut to the [pino.transport()](#pino-transport) function.
It supports the same input options:
```js
require('pino')({
Copy link
Member

Choose a reason for hiding this comment

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

do we do this require('pino') inline thing elsewhere in the docs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are 18 occurrences of require('pino') in this file actually: would you like to migrate the examples to pino?

Copy link
Member

Choose a reason for hiding this comment

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

Do they assign the result of require or do they use it directly like this example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Both declaration tbh

https://github.com/pinojs/pino/blob/5333d73773f59331c2d5c7962dec3ee00574fc6f/docs/api.md#destination-sonicboom--writablestream--string--object

I can update this example and the other ones if you tell me what do you prefer.
Is the cjs syntax not up to date for users?

docs/api.md Outdated Show resolved Hide resolved
Co-authored-by: David Mark Clements <[email protected]>
test/transport.test.js Outdated Show resolved Hide resolved
test/transport.test.js Outdated Show resolved Hide resolved
@mcollina mcollina added this to the v7.0.0 milestone Sep 8, 2021
@mcollina mcollina merged commit bd67ef9 into pinojs:master Sep 8, 2021
@github-actions
Copy link

github-actions bot commented Sep 9, 2022

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants