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: logger component is always required #713

Merged
merged 4 commits into from
Oct 15, 2019
Merged

Conversation

XVincentX
Copy link
Contributor

@XVincentX XVincentX commented Oct 15, 2019

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.

@XVincentX XVincentX marked this pull request as ready for review October 15, 2019 10:03
Copy link
Contributor

@karol-maciaszek karol-maciaszek left a 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,
Copy link
Contributor

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([])

Copy link
Contributor Author

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes it does!

@XVincentX XVincentX force-pushed the fix/components-requiredt branch from 5ed1af6 to 6502789 Compare October 15, 2019 11:13
Copy link
Contributor

@karol-maciaszek karol-maciaszek left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

@XVincentX XVincentX merged commit 3b0b140 into master Oct 15, 2019
@XVincentX XVincentX deleted the fix/components-requiredt branch October 15, 2019 13:29
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.

None yet

3 participants