-
Notifications
You must be signed in to change notification settings - Fork 897
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
fix: type regression on fastify #1246
fix: type regression on fastify #1246
Conversation
@@ -192,10 +105,98 @@ interface LoggerExtras extends EventEmitter { | |||
|
|||
declare namespace pino { | |||
//// Exported types and interfaces | |||
|
|||
interface BaseLogger { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why move it from top level into a namespace?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it never export outside and nobody can use.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can't we export it instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the old way of import is missing this exportation.
e.g.
import pino from 'pino'
pino.BaseLogger
Top-level export is mapped on line 768
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We generally want to move away from relying on namespace. See #1193 (comment)
I agree that types should be fixed if we lost the way to do something we could do before, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it should move away from namespace, but the current version is too late to do it.
Current version should support the import syntax that can be used in 7.0.0
.
The complete removal should be happen in next major 8.0.0
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that we can't remove it yet, but can we at least already provide alternative for people who want to do direct named imports? That was the gist of #1193 and it seems like the right direction.
Other than that, PR LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mention on line 768, it has the top-level export mapped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR already provide the type import for below format.
// named import
import { BaseLogger } from 'pino'
// namespace import / default import
import pino from 'pino'
pino.BaseLogger
// pino named import
import { pino } from 'pino'
pino.BaseLogger
// old P named import
import { P } from 'pino'
P.BaseLogger
If anything is missing, please let me know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for clarification!
I have tested even without my changes, it failed the test. I have no idea why it failed by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@climba03003 can you rebase? |
@kibertoad are you good with this landing? |
bc55c74
to
5834906
Compare
CI fail is not related to TypeScript changes. |
This can land as soon as @kibertoad approves. #1253 is to improve that test. |
|
||
// Interfaces | ||
export interface BaseLogger extends pino.BaseLogger {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kibertoad I think here is what you mention?
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. |
Fixes the type regression on fastify/fastify#3506