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

feat!: react-query and supplementary improvements #147

Closed
wants to merge 21 commits into from

Conversation

dalechyn
Copy link
Collaborator

Will close #146

The main motivation being that with pnpm packages
in the monorepo don't have to specify versions of
the dependencies of other packages in the monorepo.
It is also widely adopted across different monorepos.

As well as `changesets` would replace the `workspace:*`
marked versions on fly, so there won't be a need in managing
those by hands (if there was one before).

Fixes the lint issue, where in `with-next-auth`
example a lint config was not properly set up, which caused the `lint`
job to hang up waiting for stdin input.

Adds a `turbo.json` config to `auth-kit` to depend on `auth-client`,
as the build job for the first depends on the latter.
@dalechyn
Copy link
Collaborator Author

dalechyn commented Feb 28, 2024

const deadline = Date.now() + timeout;
while (Date.now() < deadline) {

This code seems incorrect as the Date.now() would be re-evaluated on the while clause check. Fixing it as well.

Moved the `Date.now()` to `now` variable to ensure the timestamp
at which the polling started is preserved correctly.
@dalechyn
Copy link
Collaborator Author

dalechyn commented Feb 28, 2024

export const watchStatus = async (client: Client, args: WatchStatusArgs): WatchStatusResponse => {
const result = await poll<StatusAPIResponse>(
client,
path,
{
timeout: args?.timeout ?? 300_000,
interval: args?.interval ?? 1000,
onResponse: args?.onResponse ?? voidCallback,
},
{ authToken: args.channelToken },
);
return unwrap(result);
};

I believe this is a bit misleading, as we don't watch the status change here. In fact, the poll function which is used, polls until success, meaning that it doesn't return multiple values as an async generator would, but rather retries the request on not satisfying response with a certain interval until a deadline is met and watch is something that watches the value being changed over time.

I'm considering to refactor this as well.

@dalechyn
Copy link
Collaborator Author

dalechyn commented Feb 28, 2024

const validateNonce = (message: SiweMessage, nonce: string): AuthClientResult<SiweMessage> => {
if (message.nonce !== nonce) {
return err(new AuthClientError("unauthorized", "Invalid nonce"));
} else {
return ok(message);
}
};
const validateDomain = (message: SiweMessage, domain: string): AuthClientResult<SiweMessage> => {
if (message.domain !== domain) {
return err(new AuthClientError("unauthorized", "Invalid domain"));
} else {
return ok(message);
}
};

Those methods should not return any value as they don't mutate it. Same concern addressed in validate.ts methods.

@dalechyn
Copy link
Collaborator Author

const deadline = Date.now() + timeout;
while (Date.now() < deadline) {

This code seems incorrect as the Date.now() would be re-evaluated on the while clause check. Fixing it as well.

Crap, just realized this is intended. Will undo in next commit.

It is a breaking change, since the hook APIs also change.
However, everything is much more reliable now, and effects are not triggered
when they don't have to.

The context was changed to simply hold the `config`, which one has now
to initiate with `createConfig`, in order to construct an `appClient`.

The rest of the state now is handled by `zustand`, specifically, the
`useProfile` and `useSignInMessage` use it.
@dalechyn dalechyn changed the title feat: react-query and supplementary improvements feat!: react-query and supplementary improvements Feb 28, 2024
@dalechyn dalechyn marked this pull request as ready for review February 28, 2024 21:58
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.github/workflows/ci.yml Outdated Show resolved Hide resolved
.gitignore Show resolved Hide resolved
Dockerfile.relay Show resolved Hide resolved
Dockerfile.relay Outdated Show resolved Hide resolved
Dockerfile.relay Outdated Show resolved Hide resolved
@dalechyn dalechyn marked this pull request as draft February 29, 2024 01:29
@dalechyn dalechyn marked this pull request as ready for review February 29, 2024 12:38
if (
signInError instanceof AuthClientError &&
signInError.errCode === "unavailable" &&
signInError.message.startsWith("Polling timed out after")
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

could be a specific errCode

[signIn]
);
const handleSuccess = useCallback((res: CompletedStatusAPIResponse) => {
console.log(res);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
console.log(res);

@dalechyn dalechyn marked this pull request as draft March 18, 2024 00:49
@dalechyn
Copy link
Collaborator Author

dalechyn commented Mar 24, 2024

I'm thinking about the implementation of useSignInMessage and useProfile hooks, probably they don't need to store the value from the response.

I believe it is a similar state management question as wagmi's connector handling.
Specifically, one could say that they could persist the return of the connectors between re-renders, but instead they have the autoConnect option that reconnects the providers (although they have cookie persistence to pass previous data until new one is hydrated https://wagmi.sh/react/guides/ssr#persistence-using-cookies).


Thus, I believe that useSignMessage should not be a client store but rather a function that re-uses query client cache to retrieve the latest used signing message.

For the useProfile – I believe it should be a query/a hook that retrieves data from the query client cache as well.

What has to be persisted though is something like a session token, optimistically with a certain deadline.

The current problem is that AFAIK the profile data is retrieved only from the pollStatusTillSuccess and is only returned in CompletedStatusResponse, therefore is only retrievable when a user undergoes through the QR Code authentication process.

What I believe should be done instead is that auth-relay should generate an access token for a user when one sucessfully undergoes the QR Code authentication, and auth-client should handle the creation of session tokens, their persistence and expiry.

With those changes applied, hooks for client data retrieval (optimistically useProfile) would not require a user to under-go QR flow, but would require an access token that might be persisted by auth-client or other client-side management implementations.

cc @horsefacts

@horsefacts horsefacts requested a review from tmm March 25, 2024 21:30
@dalechyn
Copy link
Collaborator Author

TODO: Refactor use profile and use sign in message hooks.

useProfile should make a request to relay server to retrieve the profile as after a page refresh the data is lost.

useSignInMessage should take the data from query cache.

@deodad
Copy link
Member

deodad commented Apr 16, 2024

I'm thinking about the implementation of useSignInMessage and useProfile hooks, probably they don't need to store the value from the response.

I believe it is a similar state management question as wagmi's connector handling. Specifically, one could say that they could persist the return of the connectors between re-renders, but instead they have the autoConnect option that reconnects the providers (although they have cookie persistence to pass previous data until new one is hydrated https://wagmi.sh/react/guides/ssr#persistence-using-cookies).

Thus, I believe that useSignMessage should not be a client store but rather a function that re-uses query client cache to retrieve the latest used signing message.

For the useProfile – I believe it should be a query/a hook that retrieves data from the query client cache as well.

What has to be persisted though is something like a session token, optimistically with a certain deadline.

The current problem is that AFAIK the profile data is retrieved only from the pollStatusTillSuccess and is only returned in CompletedStatusResponse, therefore is only retrievable when a user undergoes through the QR Code authentication process.

What I believe should be done instead is that auth-relay should generate an access token for a user when one sucessfully undergoes the QR Code authentication, and auth-client should handle the creation of session tokens, their persistence and expiry.

With those changes applied, hooks for client data retrieval (optimistically useProfile) would not require a user to under-go QR flow, but would require an access token that might be persisted by auth-client or other client-side management implementations.

cc @horsefacts

I don't think we want the relays to become general purpose auth servers. Session tokens, and their persistence and expiry should be up to the developer and the auth setup they're using.

@dalechyn
Copy link
Collaborator Author

I'm thinking about the implementation of useSignInMessage and useProfile hooks, probably they don't need to store the value from the response.

I believe it is a similar state management question as wagmi's connector handling. Specifically, one could say that they could persist the return of the connectors between re-renders, but instead they have the autoConnect option that reconnects the providers (although they have cookie persistence to pass previous data until new one is hydrated https://wagmi.sh/react/guides/ssr#persistence-using-cookies).

Thus, I believe that useSignMessage should not be a client store but rather a function that re-uses query client cache to retrieve the latest used signing message.

For the useProfile – I believe it should be a query/a hook that retrieves data from the query client cache as well.

What has to be persisted though is something like a session token, optimistically with a certain deadline.

The current problem is that AFAIK the profile data is retrieved only from the pollStatusTillSuccess and is only returned in CompletedStatusResponse, therefore is only retrievable when a user undergoes through the QR Code authentication process.

What I believe should be done instead is that auth-relay should generate an access token for a user when one sucessfully undergoes the QR Code authentication, and auth-client should handle the creation of session tokens, their persistence and expiry.

With those changes applied, hooks for client data retrieval (optimistically useProfile) would not require a user to under-go QR flow, but would require an access token that might be persisted by auth-client or other client-side management implementations.

cc @horsefacts

I don't think we want the relays to become general purpose auth servers. Session tokens, and their persistence and expiry should be up to the developer and the auth setup they're using.

Yep! I figured it out a bit later and need to rename some things now.

@dalechyn
Copy link
Collaborator Author

I dropped e2e test in the merge conflict on purpose. tbh a bit lazy to rewrite it right now, that effort has to be put later on.

@dalechyn
Copy link
Collaborator Author

pipelines fail because I've changed the package manager.

@dalechyn
Copy link
Collaborator Author

tbh I have some questions regarding the dalechyn@62874a0 but we can think of this later if we want this pr to ever make it lul.

@dalechyn dalechyn marked this pull request as ready for review April 17, 2024 23:28
@dalechyn dalechyn requested a review from horsefacts April 17, 2024 23:28
@dalechyn
Copy link
Collaborator Author

dalechyn commented May 15, 2024

gonna separate the efforts in this fork: https://github.com/dalechyn/fc-auth, since the changes are too massive to be merged.

@dalechyn dalechyn closed this May 15, 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.

feat: using @tanstack/react-query
3 participants