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

Settings attempts to login without credentials #6928

Closed
drewvolz opened this issue Feb 9, 2023 · 2 comments · Fixed by #6938
Closed

Settings attempts to login without credentials #6928

drewvolz opened this issue Feb 9, 2023 · 2 comments · Fixed by #6938

Comments

@drewvolz
Copy link
Member

drewvolz commented Feb 9, 2023

Is this a regression from #6899?

Upload.from.GitHub.for.iOS.MOV
@drewvolz
Copy link
Member Author

drewvolz commented Feb 9, 2023

WIP on refactoring at drew/credentials-errors

I think these changes fix most things. The only pending issue (also on our main branch) is that logout doesn't properly reset/invalidate the stored credentials.

@drewvolz
Copy link
Member Author

drewvolz commented Feb 10, 2023

@hawkrives I've spent some time looking at this and I think I've pinpointed where the issue is coming from. useCredentials calls loadCredentials which throws an error when the credentials are false. This blocks the logout state from finishing.

# source/lib/login.ts

async function loadCredentials(): Promise<SharedWebCredentials> {
	let credentials = await getInternetCredentials(SIS_LOGIN_KEY)
-	if (credentials === false) {
-		throw new NoCredentialsError()
-	}
	return credentials
}
  • This is occurring on our main branch
  • This can be tested by:
    1. Exiting early in performLogin and returning credentials without hitting the login endpoint
    2. Removing the lines shown in the above diff in login.ts

If useCredentials --> useQuery --> loadCredentials --> throws

It wouldn't get the new values. Ever. Right?

So things are invalidated, which is why we get the new correct UI after dismissing settings

  • We are throwing for no credentials in signing in, which is good, and our UI prevents us from hitting that
  • We only need to throw for no credentials when signing in (performLogin) but not when requesting the credentials
  • loadCredentials should never be allowed to throw
  • But we need to still guarantee to the consumers that "false" is an okay value

Ideas around fixing this

- async function loadCredentials(): Promise<SharedWebCredentials> {
+ async function loadCredentials(): Promise<false|SharedWebCredentials> {
	let credentials = await getInternetCredentials(SIS_LOGIN_KEY)
-	if (credentials === false) {
-		throw new NoCredentialsError()
-	}
	return credentials
}

export async function performLogin(
	credentials: Credentials | null = null,
): Promise<Credentials> {
+	const saved = credentials ?? (await loadCredentials())
+	if (!saved) {
+		throw new NoCredentialsError()
+	}

+	const {username, password} = saved

...
}

Now the challenge is to convince useCredentials that false is an okay value

...after an out of band conversation with Hawken, it looks like the magic is within the QueryFnData type

If we add "|false" to that line, and put useCredentials back without the throw, what happens?

  • TData and TError are the internal type names used by useQuery
  • TData is the type returned by queryFn, and TError is … the types that "error" can be in the result of useQuery (more or less, "what errors can this throw")

Except that TData is actually the "type returned by select:", which defaults to the identity function.

So if we pretend that we have TQueryReturn and TSelectReturn,

let query = useQuery({
queryFn: () => blah, // returns TQueryReturn
select: (data: TQueryReturn) // returns TSelectReturn

query.data: TSelectReturn

And TData is TSelectReturn

I hate how much type inference is happening in useCredentials & friends - it makes horrid errors

Nice! Looks like your suggestion works to change where our types are now expecting false!

-type QueryFnData = SharedWebCredentials
+type QueryFnData = SharedWebCredentials|false

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

Successfully merging a pull request may close this issue.

1 participant