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

Update types #237

Closed
wants to merge 5 commits into from
Closed

Update types #237

wants to merge 5 commits into from

Conversation

Joabesv
Copy link

@Joabesv Joabesv commented Nov 16, 2023

Refactoring the types for a better autocomplete of functions and plugin options. The reason for the change is because of this behavior
image

Checklist

@Joabesv Joabesv changed the title Refac/types Update types Nov 16, 2023
@Joabesv
Copy link
Author

Joabesv commented Nov 16, 2023

Sorry for not opening an issue, i saw this while coding today and just started doing it, should i open an issue or only the pr is fine?

@@ -43,10 +43,6 @@ const OAuth2Options: FastifyOAuth2Options = {
credentials: credentials,
callbackUri: 'http://localhost/testOauth/callback',
callbackUriParams: {},
generateStateFunction: () => {
Copy link
Member

Choose a reason for hiding this comment

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

why this has been removed?

Copy link
Author

Choose a reason for hiding this comment

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

it wasn't being used in any other place. Since it needs to receive a fastifyRequest, it essentially wasn't doing anything, but typescript was yelling, so I removed it!

Copy link
Member

Choose a reason for hiding this comment

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

You still need the test for this two function.
Please update to the valid syntax.

types/index.d.ts Outdated Show resolved Hide resolved
@Joabesv
Copy link
Author

Joabesv commented Nov 21, 2023

is this good to go?
cc: @climba03003 @Eomm

Copy link
Member

@gurgunday gurgunday left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@mcollina
Copy link
Member

@climba03003 are you ok the current changes?

@mcollina
Copy link
Member

@Eomm ?

Copy link
Member

@climba03003 climba03003 left a comment

Choose a reason for hiding this comment

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

It still missing the assertion of the updated types.

@Joabesv
Copy link
Author

Joabesv commented Nov 29, 2023

Sorry for disappearing here, got a bit out of time. Will be finishing this weekend!

@mcollina
Copy link
Member

Any updates?

Signed-off-by: Aras Abbasi <aras.abbasi@googlemail.com>
@Joabesv
Copy link
Author

Joabesv commented May 31, 2024

Delayed to much here, might peak up on a new pr, closing this.

@Joabesv Joabesv closed this May 31, 2024
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.

6 participants