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

feat(types): add missing typescript definitions for reporters #94

Merged
merged 3 commits into from May 7, 2020
Merged

feat(types): add missing typescript definitions for reporters #94

merged 3 commits into from May 7, 2020

Conversation

regevbr
Copy link
Contributor

@regevbr regevbr commented Apr 17, 2020

Fix #92

@regevbr
Copy link
Contributor Author

regevbr commented Apr 17, 2020

@Atinux @pi0 can you please CR this?

@regevbr
Copy link
Contributor Author

regevbr commented Apr 17, 2020

There are 3 drawback here

  1. I added typings for the log levels. The silent level is set to be Infinity in the code. But typescript doesn't provide a valid way to use it a type literal - Allow Inifinity and -Infinity as number literal types microsoft/TypeScript#32277. To overcome it I set it to be a random really high number.
    Since the value of infinity is not really used by the code, and only used to denote that if someone inputs a really high level, it will effectively not log anything, I think my solution will work, but I believe it will be better to set a reasonable high number (such as 9999) to be marked as the silent log level.

  2. I see that you expose reporters based on env (node/browser) as far as I can figure, there is no proper way to type it...

  3. In order to provide proper typings for the winston reporter, we must import it. But if someone doesn't use it, it will produce a compilation error. This error can be prevented by setting the skipLibCheck flag to true in tsconfig.json
    I usually use that flag any ways, but I guess some people will discourage it. I don't mind making the logger type there to be any if you prefer.

"skipLibCheck": true,

What do you think?

export type DebugLogLevel = 4;
export type TraceLogLevel = 5;
export type SilentLogLevel = Infinity;
export type LogLevel =
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead use an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Enums won't work well on d.ts alone as you must expose a real object to represent them. I will see if I can add it to the js and then expose it as an Enum here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed it, can you please CR it again?

@pi0
Copy link
Member

pi0 commented Apr 22, 2020

Hi @regevbr thanks for the PR. I think based on the amount of new types it is better to rewrite lib to typescript to auto-generate them.

@regevbr
Copy link
Contributor Author

regevbr commented Apr 22, 2020

Hi rewriting will take a long time and I'm no available for it right now as I'm working on other projects. Do tou think we can use those typings for now?

@pi0
Copy link
Member

pi0 commented Apr 22, 2020

Of course, we can do it until rewrite. I'm just worried about winston and log level types. Can we, for now, omit Winston and use an enum (via js) for levels?

@regevbr
Copy link
Contributor Author

regevbr commented Apr 22, 2020

Great!
Sure already implement the enum and I will drop winston

@pi0 pi0 changed the title bug: missing typescript definitions for reporters #92 feat(types): add missing typescript definitions for reporters May 7, 2020
@pi0 pi0 merged commit 4a08ef0 into unjs:master May 7, 2020
@regevbr regevbr deleted the #92 branch May 7, 2020 11:37
@regevbr
Copy link
Contributor Author

regevbr commented May 7, 2020

Thanks @pi0! when can we expect a new release with the changes?

@pi0
Copy link
Member

pi0 commented May 7, 2020

@regevbr sorry for the delay. will be released in a few minutes. Thanks again for PR ❤️ :)

@regevbr
Copy link
Contributor Author

regevbr commented May 7, 2020

Sure thing! thanks :-)

@Atinux
Copy link
Member

Atinux commented May 7, 2020

It's out: https://github.com/nuxt-contrib/consola/releases/tag/v2.12.0

Thank you @regevbr :)

@regevbr
Copy link
Contributor Author

regevbr commented May 7, 2020

Thanks @Atinux

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

missing typescript definitions for reporters
3 participants