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 PluginOptions incorrect typings #53

Merged
merged 2 commits into from
Mar 28, 2020
Merged

Conversation

felixputera
Copy link
Contributor

@felixputera felixputera commented Mar 27, 2020

Hello 👋

First of all, thanks @mcollina, @delvedor & all contributors for the awesome fastify ecosystem. I had just recently dived into it & I'm very impressed.

Moving on to the issue on hand, I found out that the TS typing provided for PluginOptions (used in fastify.register(ws, opts \*here*\) is incorrect: the typings requires both handler and options to be passed in. I looked into the docs & code to verify that it has defaults specified.

I fixed this in this PR, and had added tests to cover this use-case. Additionally, I did minor clean-up on the declaration file.

I think this addresses #47 too.

Checklist

  • run npm run test and npm run benchmark (I couldn't find npm run bench nor npm run benchmark, should the PR template be updated?)
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message and code follows Code of conduct

addtionally, tests were added to cover this change
remove unused imports & clean up dangling whitespaces
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

@mcollina
Copy link
Member

Thanks, I'm glad that you like it.

Copy link
Member

@Eomm Eomm left a comment

Choose a reason for hiding this comment

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

💪🏻

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

@mcollina mcollina merged commit 30c3ae3 into fastify:master Mar 28, 2020
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.

3 participants