Refactor AppDetailsPage to Macaw Next #3818
Conversation
🦋 Changeset detectedLatest commit: 59f1cc7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
github-actions
bot
temporarily deployed
to
storybook refactor-app-details
github-actions
bot
temporarily deployed
to
storybook refactor-app-details
| if (dataPrivacyUrl) { | ||
| return ( | ||
| <ExternalLink href={dataPrivacyUrl} target="_blank"> | ||
| <FormattedMessage {...messages.dataPrivacyDescription} /> | ||
| </ExternalLink> | ||
| ); | ||
| } else { | ||
| return <Text>{intl.formatMessage(messages.noDataPrivacyPage)}</Text>; | ||
| } | ||
|
|
||
| throw new Error("Leaking switch statement"); |
There was a problem hiding this comment.
Implementing return early is optional (non-blocking).
Throw instruction can be removed since it's unreachable.
| if (dataPrivacyUrl) { | |
| return ( | |
| <ExternalLink href={dataPrivacyUrl} target="_blank"> | |
| <FormattedMessage {...messages.dataPrivacyDescription} /> | |
| </ExternalLink> | |
| ); | |
| } else { | |
| return <Text>{intl.formatMessage(messages.noDataPrivacyPage)}</Text>; | |
| } | |
| throw new Error("Leaking switch statement"); | |
| if (!dataPrivacyUrl) { | |
| return <Text>{intl.formatMessage(messages.noDataPrivacyPage)}</Text> | |
| } | |
| return ( | |
| <ExternalLink href={dataPrivacyUrl} target="_blank"> | |
| <FormattedMessage {...messages.dataPrivacyDescription} /> | |
| </ExternalLink> | |
| ); |
Was this error necessary? Seems strange 🤔
There was a problem hiding this comment.
Yes. Component should render only for cases I want it to. If none of conditions are met, this is a bug in the code. Should not happen. Just like leaky switch/cases. Hence throwing error to quickly see that something wrong happened. If I return "null" here, we will never now if there is a bug
There was a problem hiding this comment.
That's if/else with returning statements inside. Exception throw is unreachable
| } | ||
|
|
||
| if (aboutApp) { | ||
| return <ReactMarkdown source={aboutApp ?? ""} />; |
There was a problem hiding this comment.
| return <ReactMarkdown source={aboutApp ?? ""} />; | |
| return <ReactMarkdown source={aboutApp} />; |
| return <Text>{intl.formatMessage(messages.noDataPrivacyPage)}</Text>; | ||
| } | ||
|
|
||
| throw new Error("Leaking switch statement"); |
There was a problem hiding this comment.
suggestion: shouldn't it be logged into sentry?
There was a problem hiding this comment.
Won't sentry automatically do that? Its unhandled on purpose
There was a problem hiding this comment.
I added error boundary and explicitely called Sentry
github-actions
bot
temporarily deployed
to
storybook refactor-app-details
closes #...
This is part of saleor/apps#569
This PR:
Next steps will be adding more features to this page, like removing permissions
Screenshots
Pull Request Checklist
data-test-idare added for new elementsTest environment config
API_URI=https://automation-dashboard.staging.saleor.cloud/graphql/
APPS_MARKETPLACE_API_URI=https://apps.staging.saleor.io/api/v2/saleor-apps
Do you want to run more stable tests?
To run all tests, just select the stable checkbox. To speed up tests, increase the number of containers. Tests will be re-run only when the "run e2e" label is added.
CONTAINERS=1