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

fix: type regression on fastify #1246

Merged

Conversation

climba03003
Copy link
Contributor

Fixes the type regression on fastify/fastify#3506

@@ -192,10 +105,98 @@ interface LoggerExtras extends EventEmitter {

declare namespace pino {
//// Exported types and interfaces

interface BaseLogger {
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for clarification!

@climba03003
Copy link
Contributor Author

I have tested even without my changes, it failed the test.

I have no idea why it failed by @types/jsdom type check and it is something that we never import.

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

@mcollina
Copy link
Member

mcollina commented Dec 1, 2021

@climba03003 can you rebase?
@kibertoad fixed this in #1251.

@mcollina
Copy link
Member

mcollina commented Dec 1, 2021

@kibertoad are you good with this landing?

@climba03003 climba03003 force-pushed the fix--type-regression-on-fastify branch from bc55c74 to 5834906 Compare December 1, 2021 08:15
@climba03003
Copy link
Contributor Author

CI fail is not related to TypeScript changes.

@mcollina
Copy link
Member

mcollina commented Dec 1, 2021

This can land as soon as @kibertoad approves. #1253 is to improve that test.


// Interfaces
export interface BaseLogger extends pino.BaseLogger {}
Copy link
Contributor Author

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?

@kibertoad kibertoad merged commit ec9b0b5 into pinojs:master Dec 1, 2021
@github-actions
Copy link

github-actions bot commented Dec 2, 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 Dec 2, 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.

None yet

3 participants