Handle GitHub token revocation #1849
Conversation
ghost
removed their request for review
| }) | ||
| }) | ||
| } | ||
| return initApolloClient |
There was a problem hiding this comment.
I think we can save all consumers of useApolloClient a line or two by doing something like this:
export const useApolloClient = () => {
const { api, appState } = useAragonApi()
if (appState.github.token === null) return null
return new ApolloClient({ ... })
}Note that there's also no reason to pass token in as an argument anymore, now that this is a hook!
Do you think this would work? Is there a reason to make this return an initApolloClient function, rather than null or an actual client?
| scope: null, | ||
| token: null, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Let's add an else with console.error(error), to keep the functionality we had before and make sure these errors are still visible in the console
e18r
force-pushed
the
projects-token-revoked
branch
from
b5e8b4a to
3ab107c
Compare
|
Hey @chadoh, this PR is ready for another round of review. Thanks! |
When a GitHub request is made with a revoked token, the end user is informed and asked to sign in again.
Previously, in order to use the Github API client, you had to: 1. Get the GitHub token from the appState 2. Call the `useApolloClient` hook and get the `initApolloClient` function 3. Call the `initApolloClient` function with the GitHub token as an argument and get the client But since `useApolloClient` is a hook, this commit simplifies it so that the appState is accessed directly from within the hook, and the actual API client is returned directly, so that the above steps get simplified to: 1. Call the `useApolloClient` hook and get the client
e18r
force-pushed
the
projects-token-revoked
branch
from
3ab107c to
7284235
Compare
|
Rebased with |
There was a problem hiding this comment.
This looks really nice! I see a couple more small improvements we could make, which would not require the "request changes" status, but I also see something else...
The IssueDetail page doesn't work! I haven't looked into why, but it works on dev and is broken on this branch.
| import PropTypes from 'prop-types' | ||
| import { Button, EmptyStateCard, GU, Text, useTheme } from '@aragon/ui' | ||
| import { EmptyWrapper } from '../Shared' | ||
| import revoked from '../../assets/revoked.svg' |
There was a problem hiding this comment.
Can we export a component, instead of raw SVG? Like we did for IconDoubleCheck in Allocations
|
|
||
| const Illustration = () => <img src={revoked} alt="" /> | ||
|
|
||
|
|
There was a problem hiding this comment.
our linter doesn't complain about two line breaks in a row so I guess I have to 🙃
| github: { token } = { token: null } | ||
| } | ||
| } = useAragonApi() | ||
| if (token === null) return null |
There was a problem hiding this comment.
This is all a bit weird to read. How about:
const { api, appState: { github } } = useAragonApi()
if (!github || !github.token) return nullThen we'll have to use github.token elsewhere, but I think that's a good balance of readability here vs terseness there.
Fixes #654
When a GitHub request is made with a revoked token, the end user is informed and asked to sign in again.