feat: improve role detection #159
Conversation
| required, | ||
| }: { required?: boolean } = {}): | ||
| | { address: Address; role: UserRole } | ||
| | { address?: Address; role?: UserRole } { |
There was a problem hiding this comment.
I don't love how this function works. When using something like this I would expect it to return null instead an object with undefined inside.
There could even be 2 hooks, one that returns { address: Address; role: UserRole }| null dependeng on if the users is logged in or not, and another one that returns { address: Address; role: UserRole } or fails.
There was a problem hiding this comment.
That's exactly how it works! If required is false, then all parameters are optional. I agree we could make it or | null the entire object instead of each value but it's the same behaviour.
| useEffect(() => { | ||
| return watchAccount(config, { | ||
| onChange(account, prevAccount) { | ||
| if (account.address !== prevAccount.address && account.address !== undefined) { |
There was a problem hiding this comment.
why we don't do anything if the acc.address is undefined? Shouldn't the cookie be clear?
There was a problem hiding this comment.
We can do that, but it shouldn't affect app behaviour.
Update: severely improved role changer, with zod validation and correctly deleting cookies.
There was a problem hiding this comment.
If we don't, I would love a comment explaining why it's not needed. I feel that it's not obvious.
|
|
||
| export async function action({ request }: ActionFunctionArgs) { | ||
| const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
| await sleep(5000); //FIXME remove |
There was a problem hiding this comment.
This can be removed :)
There was a problem hiding this comment.
We should definitely add a CI that searches for FIXMEs 🙄
| required, | ||
| }: { required?: boolean } = {}): | ||
| | { address: Address; role: UserRole } | ||
| | { address?: Address; role?: UserRole } { |
There was a problem hiding this comment.
That's exactly how it works! If required is false, then all parameters are optional. I agree we could make it or | null the entire object instead of each value but it's the same behaviour.
| useEffect(() => { | ||
| return watchAccount(config, { | ||
| onChange(account, prevAccount) { | ||
| if (account.address !== prevAccount.address && account.address !== undefined) { |
There was a problem hiding this comment.
We can do that, but it shouldn't affect app behaviour.
Update: severely improved role changer, with zod validation and correctly deleting cookies.
|
|
||
| export async function action({ request }: ActionFunctionArgs) { | ||
| const sleep = (ms: number) => new Promise((resolve) => setTimeout(resolve, ms)); | ||
| await sleep(5000); //FIXME remove |
There was a problem hiding this comment.
We should definitely add a CI that searches for FIXMEs 🙄
No description provided.