Skip to content

express-winston: relax types on meta object#25036

Merged
RyanCavanaugh merged 2 commits intoDefinitelyTyped:masterfrom
dylanscott:master
Apr 17, 2018
Merged

express-winston: relax types on meta object#25036
RyanCavanaugh merged 2 commits intoDefinitelyTyped:masterfrom
dylanscott:master

Conversation

@dylanscott
Copy link
Copy Markdown
Contributor

@dylanscott dylanscott commented Apr 16, 2018

Relaxes the type of the metadata object in express-winston. Winston's own typings use any for the meta object and there are cases where you may want nested metadata. In my case I'm using the dynamic meta function to set http request metadata in a specific format to be consumed by a specific transport and can't express this with the current types.

  • Use a meaningful title for the pull request. Include the name of the package modified.
  • Test the change in your own code. (Compile and run.)
  • Add or edit tests to reflect the change. (Run with npm test.)
  • Follow the advice from the readme.
  • Avoid common mistakes.
  • Run npm run lint package-name (or tsc if no tslint.json is present).

If changing an existing definition:

@typescript-bot
Copy link
Copy Markdown
Contributor

typescript-bot commented Apr 16, 2018

@dylanscott Thank you for submitting this PR!

🔔 @bricka - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

Copy link
Copy Markdown
Contributor

@bricka bricka left a comment

Choose a reason for hiding this comment

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

First of all, awesome thanks for all of these improvements!

I'm a bit confused by this one. According to the express-winston docs, this needs to be an object:
https://github.com/bithavoc/express-winston#options

If you look at the Winston code itself, the examples also use an object here:
https://github.com/winstonjs/winston/blob/master/examples/metadata.js

I'm a bit inclined to say that the winston types should be updated to expect an object as opposed to making this accept any.

What do you think?

@dylanscott
Copy link
Copy Markdown
Contributor Author

That's a good point, the object type is probably better here. I think the main thing I was running into is that { [key: string]: string } does not allow for nested metadata objects, which seem to be supported in winston. I can change the type to object.

@typescript-bot
Copy link
Copy Markdown
Contributor

🔔 @bricka - Thanks for your review of this PR! Can you please look at the new code and update your review status if appropriate?

@typescript-bot
Copy link
Copy Markdown
Contributor

A definition author has approved this PR ⭐️. A maintainer will merge this PR shortly. If it shouldn't be merged yet, please leave a comment saying so and we'll wait. Thank you for your contribution to DefinitelyTyped!

@RyanCavanaugh RyanCavanaugh merged commit d12aef0 into DefinitelyTyped:master Apr 17, 2018
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.

4 participants