-
Notifications
You must be signed in to change notification settings - Fork 362
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: logger component is always required #713
Conversation
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.
README has outdated example + a comment on the default params
@@ -14,7 +14,7 @@ export const createServer = (operations: IHttpOperation[], opts: IPrismHttpServe | |||
const { components, config } = opts; | |||
|
|||
const server = fastify({ | |||
logger: (components && components.logger) || createLogger('HTTP SERVER'), | |||
logger: components.logger, |
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 think we should lean towards having more default parameters. Ideally, a user should be able to spin up the server with just: createServer([])
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.
The final user will always be able. They use the public client which has everything defaulted so they'll be able to use it with no parameters at all.
On the other hand, this is the internal client that I am using to create the hosted Prism version and this one requires this stuff.
Does that clarify?
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.
Yes it does!
5ed1af6
to
6502789
Compare
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! 👍
It turns out that although we're claiming that the
components
object is not required, it is indeed required because the logger instance is always required.This PR fixes this so that when using the http raw client package to have the hosted Prism version I do not incur in the problem of having something required not passed because of TypeScript not complaining.