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

Improve TS types #1099

Merged
merged 7 commits into from Aug 27, 2021
Merged

Improve TS types #1099

merged 7 commits into from Aug 27, 2021

Conversation

kibertoad
Copy link
Contributor

@kibertoad kibertoad commented Aug 26, 2021

While working on fastify/fastify#3101 I've noticed following things:

  1. Logger is not correct, as all members of an object must conform to the string index signature (see https://stackoverflow.com/questions/57371839/merge-typescript-record-or-dictionary-like-type-with-fixed-key-typings)
  2. BaseLogger is not so base, better minimal logger typing would be this: https://github.com/fastify/fastify/blob/6ded7ffd767ea0348159435a9d7119a0de859dff/types/logger.d.ts#L45
  3. Dropped some obsolete typings for code that does not seem to be present anymore

Fix index type
@kibertoad kibertoad requested a review from a team August 26, 2021 17:30
@@ -71,7 +71,7 @@
"bunyan": "^1.8.14",
"docsify-cli": "^4.4.1",
"eslint": "^7.17.0",
"eslint-config-standard": "^16.0.2",
"eslint-config-standard": "^16.0.3",
Copy link
Contributor Author

@kibertoad kibertoad Aug 26, 2021

Choose a reason for hiding this comment

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

npm 7 was screaming at me about failing to resolve dependencies unless I bumped this.

@@ -50,7 +50,6 @@ declare namespace P {
/**
* Holds the current log format version (as output in the v property of each log record).
*/
const LOG_VERSION: number;
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 no longer seems to exist


interface BaseLogger extends EventEmitter {
/**
Copy link
Contributor Author

@kibertoad kibertoad Aug 26, 2021

Choose a reason for hiding this comment

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

none of these fields seem to exist anymore on a logger instance (version is exposed as a separate exported constant, though)

@kibertoad kibertoad changed the title Fix index type Improve TS types Aug 26, 2021
@@ -870,6 +806,63 @@ declare namespace P {
* Noop function.
*/
silent: LogFn;
}

interface LoggerExtras extends EventEmitter {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not a huge fan of the name, open for better suggestions

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

@kibertoad kibertoad merged commit 3937987 into master Aug 27, 2021
@kibertoad kibertoad deleted the feat/type-cleanup branch August 27, 2021 05:29
@kibertoad
Copy link
Contributor Author

@mcollina Any chance you could release new RC?

@kibertoad
Copy link
Contributor Author

@mcollina Are there any pending changes before we can publish a new RC?

@github-actions
Copy link

github-actions bot commented Sep 9, 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 Sep 9, 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

2 participants