Bind pino instance to prettifier

[SkeLLLa]

This PR passes pino instance to prettifier function.

It can be used to access configured levels, for example, here https://github.com/pinojs/pino-pretty/blob/master/lib/colors.js#L32 levels are hardcoded and if there are customLevels setup, then it will not work correctly.

Closes #720.

PS: I'm not a fan of tap, so tests might look not right.

[mcollina]

LGTM

[davidmarkclements]

can we avoid doing this? It will slow down init time

[SkeLLLa]

This is called only once and the difference between Object.assign and {} will be nanoseconds. So Comparing to other code and node itself there will be less than statistical uncertainty.

[mcollina]

This is just run once.. this might be relevant only for serverless if at all.

[mcollina]

Actually this has a side effect, as all the definitions of the hidden class would not be done in the same place.
Can you revert?

(Good spot @davidmarkclements).

[SkeLLLa]

If instance is needed to be passed to normalize function then it should be defined before calling normalize. And normalize returns opts that are used for constructing the instance. So I don't know what's the way of avoiding this.

[mcollina]

I think this might not be needed at all.

In

pino/lib/tools.js

Line 174 in cf6cf48

lastLogger: null,

you have the last logger object used. You might just call

pino/lib/tools.js

Line 237 in cf6cf48

const formatted = pretty(typeof redact === 'function' ? redact(obj) : obj)

with pretty.call(this.lastLogger, ...) instead.

[jsumners]

@SkeLLLa how about resolving this so we can merge?

[davidmarkclements]

.

[davidmarkclements]

That’s assuming that everyone uses child loggers instead of new instances. I’ve seen the latter in the wild

…

On Fri, 4 Oct 2019 at 14:48, Matteo Collina ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pino.js <#721 (comment)> : > @@ -96,7 +97,7 @@ function pino (...args) { assertDefaultLevelFound(level, customLevels, useOnlyCustomLevels) const levels = mappings(customLevels, useOnlyCustomLevels) - const instance = { + Object.assign(instance, { This is just run once.. this might be relevant only for serverless if at all. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#721?email_source=notifications&email_token=AAJCWPAN4VSBVXXSKMNR2WLQM43S3A5CNFSM4I5AVO32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCG5LS7Q#discussion_r331483001>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJCWPGUIDQEDO6I6NJKXZTQM43S3ANCNFSM4I5AVO3Q> .

[mcollina]

That’s assuming that everyone uses child loggers instead of new instances.
I’ve seen the latter in the wild

I've seen that happen only at module scope/maybe 10 or 20 times within a project. In other terms, this does not sit in a hot path for me.

[davidmarkclements]

Fair enough - do we know why we’re doing this though? I don’t fully see the reason

…

On Fri, 4 Oct 2019 at 14:55, Matteo Collina ***@***.***> wrote: That’s assuming that everyone uses child loggers instead of new instances. I’ve seen the latter in the wild I've seen that happen only at module scope/maybe 10 or 20 times within a project. In other terms, this does not sit in a hot path for me. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#721?email_source=notifications&email_token=AAJCWPEIGMM4XJSYVMYDAOLQM44KHA5CNFSM4I5AVO32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEALRRTY#issuecomment-538384591>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJCWPAAZFUY5WOXLHEYCYDQM44KHANCNFSM4I5AVO3Q> .

[davidmarkclements]

Oh yeah that too Hahahah

…

On Fri, 4 Oct 2019 at 14:58, Matteo Collina ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pino.js <#721 (comment)> : > @@ -96,7 +97,7 @@ function pino (...args) { assertDefaultLevelFound(level, customLevels, useOnlyCustomLevels) const levels = mappings(customLevels, useOnlyCustomLevels) - const instance = { + Object.assign(instance, { Actually this has a side effect, as all the definitions of the hidden class would not be done in the same place. Can you revert? (Good spot @davidmarkclements <https://github.com/davidmarkclements>). — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#721?email_source=notifications&email_token=AAJCWPA4M6BOOFJD2YBNDGTQM44YTA5CNFSM4I5AVO32YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCG5M4MI#discussion_r331487012>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJCWPHKUPDJ7IWNOSAAEBLQM44YTANCNFSM4I5AVO3Q> .

[jsumners]

@davidmarkclements have your concerns been addressed?

[davidmarkclements]

@jsumners yeah lgtm

[github-actions]

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.