ref(js): Convert App to a functional component #28936
Conversation
| loading: false, | ||
| error: false, | ||
| needsUpgrade: ConfigStore.get('user')?.isSuperuser && ConfigStore.get('needsUpgrade'), | ||
| newsletterConsentPrompt: ConfigStore.get('user')?.flags?.newsletter_consent_prompt, |
There was a problem hiding this comment.
We no longer store either of these in state, since the component receives updates from using withConfig.
| const data = await api.requestPromise('/organizations/', {query: {member: '1'}}); | ||
| OrganizationsStore.load(data); | ||
| } catch { | ||
| // TODO: do something? |
There was a problem hiding this comment.
So we used to have a setState({error: true}) here... but we actually just never did anything with it. Ctrl+F state in this PR and you won't find any mention of error.
We probably should do something here though 🤔 maybe just log to Sentry
There was a problem hiding this comment.
yes. I think at least log it to Sentry
There was a problem hiding this comment.
I wouldn't log all the errors though. For example 401 errors are not worth sending back to sentry.
There was a problem hiding this comment.
I will follow up with this, since right now it isn't doing anything either, this comment just makes it more explicit.
| } | ||
| data?.problems?.forEach?.(problem => { | ||
| const {id, message, url} = problem; | ||
| const type = problem.severity === 'critical' ? 'error' : 'warning'; |
There was a problem hiding this comment.
This used to be a getAlertTypeForProblem function. I got rid of it since it could be this single line conditional.
| if (config.user !== undefined) { | ||
| newState.user = config.user; | ||
| // The app is running in local SPA mode | ||
| if (!DEPLOY_PREVIEW_CONFIG && EXPERIMENTAL_SPA) { |
There was a problem hiding this comment.
I broke up this conditional from using else if to make it marginally more readable.
| }; | ||
| function clearNewsletterConsent() { | ||
| const flags = {...config.user.flags, newsletter_consent_prompt: false}; | ||
| ConfigStore.set('user', {...config.user, flags}); |
There was a problem hiding this comment.
Update the ConfigStore and we don't have to maintain that state here in the component :)
static/app/views/app/index.tsx
Outdated
|
|
||
| return this.props.children; | ||
| } | ||
| const NewsletterConsent = lazy(() => import('app/views/newsletterConsent')); |
There was a problem hiding this comment.
I converted NewsletterConsent to a lazy-load. I will need to actually verify that this works.
| } | ||
|
|
||
| export default withApi(withConfig(App)); | ||
| export default withConfig(App); |
There was a problem hiding this comment.
Would be nice if the useLegacyStore could work for ConfigStore, but unfortunately it does not because it doesn't match the interface.
| const data = await api.requestPromise('/organizations/', {query: {member: '1'}}); | ||
| OrganizationsStore.load(data); | ||
| } catch { | ||
| // TODO: do something? |
There was a problem hiding this comment.
yes. I think at least log it to Sentry
| } | ||
|
|
||
| fetchGuides(); | ||
| } | ||
| data?.problems?.forEach?.(problem => { |
There was a problem hiding this comment.
should we move it to inside the try-catch? because if an error happens the data will be null anyway. can we also cast the data with a better type other than any?
There was a problem hiding this comment.
I think I would rather the try/catch only catch API errors
| <LoadingIndicator triangle> | ||
| {t('Getting a list of all of your organizations.')} | ||
| </LoadingIndicator> | ||
| <Suspense fallback={null}> |
There was a problem hiding this comment.
nice! never used suspense before . should we pass a loading indicator in the fallback?
There was a problem hiding this comment.
I think it's okay for now, it loads very fast
e23d56c to
379b7c6
Compare
379b7c6 to
91750cd
Compare
This should simplify the component quite a bit.
Still WIP (this will fail tests rn), requires #28933. But am happy to have some preliminary review.