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: type issue on the dbAuth generated pages #3068

Merged
merged 6 commits into from Jul 21, 2021
Merged

Conversation

Albert-Gao
Copy link
Contributor

close #3067

useEffect(() => {
usernameRef.current.focus()
}, [])

const onSubmit = async (data) => {
const response = await logIn({ ...data })
const response: any = await logIn({ ...data })
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just a quick fix, the decent fix should be proper type the logIn method, but does this logIn being used for all scenarios and some of them do return while some of them are not? That gonna make it more interesting...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we need to type the response here, the type can probably be inferred from the login method - which I suppose currently is any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, should be inferred, the current type is void which causes the problem.

this any is just for removing that annoying red underline in the IDE xD,

Copy link
Collaborator

Choose a reason for hiding this comment

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

Gotcha - let me check the typings for useAuth :)

Copy link
Collaborator

@dac09 dac09 Jul 20, 2021

Choose a reason for hiding this comment

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

Albert, what do you think of modifying the return types in https://github.com/redwoodjs/redwood/blob/main/packages/auth/src/AuthProvider.tsx#L23-L25 to be Promise<any> or Promise<unknown>?

The challenge here is that we don't know the exact type - because it varies per auth provider. I have plans to improve these typings, but will have to wait a little

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea, that would be a better fix since this is the source. Promise<any> would be better because unknown means the we have to cast it when use it.

We can just let that function return Promise<any> and properly type it when the API design is finalized.

@thedavidprice thedavidprice changed the title Fixed the styling issue on the auth pages Fix: type issue on the dbAuth generated pages Jul 19, 2021
@cannikin
Copy link
Member

I'd love to approve this, but I would have no idea if this fix is working or not (types are the bane of my existence). Can I substitute @dac09 for me? :)

@thedavidprice thedavidprice requested review from dac09 and removed request for cannikin July 19, 2021 23:04
@thedavidprice thedavidprice added this to the next-release-priority milestone Jul 19, 2021
@redwoodjs-bot redwoodjs-bot bot moved this from New issues to In progress (priority) in Current-Release-Sprint Jul 19, 2021
@@ -22,13 +22,13 @@ const SignupPage = () => {
}, [isAuthenticated])

Copy link
Collaborator

Choose a reason for hiding this comment

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

Same comments as login.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

@thedavidprice thedavidprice requested a review from dac09 July 20, 2021 15:28
Copy link
Collaborator

@dac09 dac09 left a comment

Choose a reason for hiding this comment

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

Awaiting changes to AuthProvider type

@Albert-Gao
Copy link
Contributor Author

Done :) Sorry was sleeping yesterday @dac09

@dac09
Copy link
Collaborator

dac09 commented Jul 20, 2021

Haha no worries @Albert-Gao - just marked the PR to stay organised. Appreciate this - will review in the morning after my sleep :)

@Albert-Gao
Copy link
Contributor Author

image

Tried, updated the type to Promise<any> solved the problem

Current-Release-Sprint automation moved this from In progress (priority) to Ready to merge Jul 21, 2021
@dac09 dac09 merged commit 2b092d4 into redwoodjs:main Jul 21, 2021
Current-Release-Sprint automation moved this from Ready to merge to Done Jul 21, 2021
@dac09
Copy link
Collaborator

dac09 commented Jul 21, 2021

Cheers @Albert-Gao! 🚀

@Albert-Gao
Copy link
Contributor Author

My pleasure 😀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

The scaffold template for Auth page has some Typing issues
4 participants