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 typescript declaration for app.use with async functions #126

Merged
merged 1 commit into from
Feb 11, 2021

Conversation

tellnes
Copy link
Contributor

@tellnes tellnes commented Feb 10, 2021

Checklist

  • run npm run test and npm run benchmark
    • npm run benchmark fails with missing script: benchmark
  • tests and/or benchmarks are included
    • not relevant
  • documentation is changed or added
    • not relevant
  • commit message and code follows the Developer's Certification of Origin
    and the Code of conduct

Btw, the test fails with an listen EADDRINUSE: address already in use :::3000 if there is something else listening on port 3000 where the tests is run.

@climba03003
Copy link
Member

climba03003 commented Feb 11, 2021

I think it really doesn't matter to the return value to be void or Promise<void>
It is valid for void itself only.
Typescript Playground
image

@tellnes
Copy link
Contributor Author

tellnes commented Feb 11, 2021

It does matter if you are running linting with the @typescript-eslint/no-misused-promises rule enabled.

But I think it should be unknown instead of void. That will allow you to use promises which returns something also. That will be fine since avvio seems to ignores any return value. I will update the PR.

@tellnes
Copy link
Contributor Author

tellnes commented Feb 11, 2021

PR updated. Changed return value from void | Promise<void> to unknown | Promise<unknown>

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.

Would you mind to add a test in https://github.com/fastify/avvio/tree/master/test/types for this?

@tellnes
Copy link
Contributor Author

tellnes commented Feb 11, 2021

Test case added. I did also remove Promise<unknown>. That is not needed anymore after I also changed the non-async part from void to unknown in an earlier change. The change is now just to change it from void to unknown.

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 14a2861 into fastify:master Feb 11, 2021
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