Remove deprecated API and options in v7 option. #1249
Conversation
100% code coverage back.
mcollina
changed the title
|
@kibertoad could you take a look why TS is failing badly? |
|
should we create a v8 branch and merge to that? |
| warning.create(warnName, 'PINODEP008', 'prettyPrint is deprecated, look at https://github.com/pinojs/pino-pretty for alternatives.') | ||
|
|
||
| warning.create(warnName, 'PINODEP009', 'The use of pino.final is discouraged in Node.js v14+ and not required. It will be removed in the next major version') | ||
| // warning.create(warnName, 'PINODEP010', 'A new deprecation') |
There was a problem hiding this comment.
This is on purpose to avoid deleting this file.
There was a problem hiding this comment.
is the file needed? if not we should remove it - if we ever need it again it's in the commit history right?
There was a problem hiding this comment.
It's easier to keep it and update it as necessary.
| @@ -3,8 +3,6 @@ | |||
| const warning = require('fastify-warning')() | |||
| module.exports = warning | |||
|
|
|||
| const warnName = 'PinoWarning' | |||
There was a problem hiding this comment.
is this file required anywhere else now? if not, remove?
There was a problem hiding this comment.
We'll definitely need it at some point in the future, better to keep.
There was a problem hiding this comment.
for me it's a loose end - maybe this whole concept should be a separate module or something
It should be a |
| @@ -510,16 +316,6 @@ function buildFormatters (level, bindings, log) { | |||
| } | |||
| } | |||
|
|
|||
| function setMetadataProps (dest, that) { | |||
| @@ -503,21 +503,17 @@ test('flushSync', function (t) { | |||
| '_' + Math.random().toString(36).substr(2, 9) | |||
| ) | |||
| const destination = pino.destination({ dest: tmp, sync: false, minLength: 4096 }) | |||
| const log = pino({ level: 'info' }, multistream([{ level: 'info', stream: destination }])) | |||
| const stream = multistream([{ level: 'info', stream: destination }]) | |||
There was a problem hiding this comment.
pino v8 side note: can we merge multistream and transport APIs in a way where the multistream same-thread logic is used based on available threads (or a force option)
There was a problem hiding this comment.
I suggest to park this till v9, it's better to ship what we have and improve in the next iteration.
I'm on it. |
|
@mcollina Please don't laugh, but I think this one was actually a Jest issue. Fixed in a non-elegant way for now because Jest requires it, but shouldn't matter because we only use tsconfig for local testing anyway. Not sure why it only triggered now. They promise to do better in v28: jestjs/jest#12098 |
|
@mcollina Is there anything else that you want to see in this PR, or we can merge after CI is green? |
|
Can you add Node v18 and drop v17? |
|
@jsumners you good with pino@8 being a tiny update just to drop node v12? |
Yes. |
|
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. |
100% code coverage back.
This is the start of a v8 planning. We might want to do this sooner than expected and roll it out with Fastify v4.
Removes:
prettyPrintpino.final()