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

refactor: review typescript signature #164

Merged
merged 7 commits into from
Oct 10, 2023
Merged

refactor: review typescript signature #164

merged 7 commits into from
Oct 10, 2023

Conversation

Puppo
Copy link
Contributor

@Puppo Puppo commented Oct 7, 2023

@Eomm and @mateonunez, I reviewed a little the typescript interface and removed the getOrama method.
In this way, there is only one decorator, and if someone wants to use typescript, using the withOrama<T>() get the FastifyInstance with the correct type and with the orama decorator set up.

index.js Show resolved Hide resolved
@@ -31,13 +31,17 @@ type FastifyOramaPluginOptions = {

declare const fastifyOrama: FastifyPluginCallback<FastifyOramaPluginOptions>

interface OramaApi<T> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like it

test/index.test.js Show resolved Hide resolved
@@ -1,9 +1,9 @@
import { expectType } from 'tsd'
import {expectType} from 'tsd'
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the linting here?

Suggested change
import {expectType} from 'tsd'
import { expectType } from 'tsd'

@Puppo Puppo requested a review from Eomm October 9, 2023 06:43
Copy link
Owner

@mateonunez mateonunez left a comment

Choose a reason for hiding this comment

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

LGTM!

@Puppo
Copy link
Contributor Author

Puppo commented Oct 9, 2023

@mateonunez and @Eomm seems the CI is stick! I don't have any permission on it, but can one of you check it or restart the job?

@mateonunez
Copy link
Owner

@mateonunez and @Eomm seems the CI is stick! I don't have any permission on it, but can one of you check it or restart the job?

The action gets stuck with Node v20 here:

image

@mateonunez
Copy link
Owner

CI passed.

@Puppo
Copy link
Contributor Author

Puppo commented Oct 10, 2023

Okay, I don't know how. I had the same problem locally with node 20.
But okay 👍
Thanks @mateonunez

@mateonunez
Copy link
Owner

Let's see what @Eomm says, then I think we can merge it into the cjs branch.

@Eomm Eomm merged commit 23b4c62 into mateonunez:cjs Oct 10, 2023
3 checks passed
mateonunez pushed a commit that referenced this pull request Oct 17, 2023
* breaking: migrate to cjs

* fix: typescript interface

* refactor: review typescript signature (#164)

---------

Co-authored-by: Luca Del Puppo <Puppo@users.noreply.github.com>
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