Skip to content

Commit

Permalink
ref: Allow conditional loading in ReleaseSeries (#83506)
Browse files Browse the repository at this point in the history
Adds an `enabled` prop to `ReleaseSeries`. This makes it _much_ easier
to conditinally render the component! In a few places in the app we do
this kind of shenanigan:

```jsx
...

if (needsReleases) {
  return <ReleaseSeries blah blah
            <MyComponent
} else { 
  return <MyComponent
}
```

If `MyComponent` is a big chunk of JSX, this is a lot of very gross
duplication. Instead, I'm following the same idea as our data hooks. An
`enabled` prop can turn `ReleaseSeries` fetching on and off. Then,
`<MyComponent` can check whether it got releases or not. No fuss, no
muss.
  • Loading branch information
gggritso authored Jan 15, 2025
1 parent 5034d9b commit ac0c4ac
Show file tree
Hide file tree
Showing 2 changed files with 56 additions and 9 deletions.
42 changes: 41 additions & 1 deletion static/app/components/charts/releaseSeries.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import {Fragment} from 'react';
import {OrganizationFixture} from 'sentry-fixture/organization';
import {RouterFixture} from 'sentry-fixture/routerFixture';

import {render, screen, waitFor} from 'sentry-test/reactTestingLibrary';
import {act, render, screen, waitFor} from 'sentry-test/reactTestingLibrary';

import type {ReleaseSeriesProps} from 'sentry/components/charts/releaseSeries';
import ReleaseSeries from 'sentry/components/charts/releaseSeries';
Expand Down Expand Up @@ -59,6 +59,46 @@ describe('ReleaseSeries', function () {
expect(releasesMock).not.toHaveBeenCalled();
});

it('does not fetch releases if not enabled', function () {
render(
<ReleaseSeries {...baseSeriesProps} organization={organization} enabled={false}>
{renderFunc}
</ReleaseSeries>
);

expect(releasesMock).not.toHaveBeenCalled();
});

it('fetches releases if becomes enabled', async function () {
const {rerender} = render(
<ReleaseSeries {...baseSeriesProps} organization={organization} enabled={false}>
{renderFunc}
</ReleaseSeries>
);

expect(releasesMock).not.toHaveBeenCalled();

rerender(
<ReleaseSeries {...baseSeriesProps} organization={organization} enabled>
{renderFunc}
</ReleaseSeries>
);

await act(tick);

expect(releasesMock).toHaveBeenCalledTimes(1);

rerender(
<ReleaseSeries {...baseSeriesProps} organization={organization} enabled={false}>
{renderFunc}
</ReleaseSeries>
);

await act(tick);

expect(releasesMock).toHaveBeenCalledTimes(1);
});

it('fetches releases if no releases passed through props', async function () {
render(<ReleaseSeries {...baseSeriesProps}>{renderFunc}</ReleaseSeries>);

Expand Down
23 changes: 15 additions & 8 deletions static/app/components/charts/releaseSeries.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ export interface ReleaseSeriesProps extends WithRouterProps {
start: DateString;
theme: Theme;
emphasizeReleases?: string[];
enabled?: boolean;
memoized?: boolean;
period?: string | null;
preserveQueryParams?: boolean;
Expand All @@ -105,25 +106,31 @@ class ReleaseSeries extends Component<ReleaseSeriesProps, State> {

componentDidMount() {
this._isMounted = true;
const {releases} = this.props;
const {releases, enabled = true} = this.props;

if (releases) {
// No need to fetch releases if passed in from props
this.setReleasesWithSeries(releases);
return;
}

this.fetchData();
if (enabled) {
this.fetchData();
}
}

componentDidUpdate(prevProps) {
const {enabled = true} = this.props;

if (
!isEqual(prevProps.projects, this.props.projects) ||
!isEqual(prevProps.environments, this.props.environments) ||
!isEqual(prevProps.start, this.props.start) ||
!isEqual(prevProps.end, this.props.end) ||
!isEqual(prevProps.period, this.props.period) ||
!isEqual(prevProps.query, this.props.query)
(!isEqual(prevProps.projects, this.props.projects) ||
!isEqual(prevProps.environments, this.props.environments) ||
!isEqual(prevProps.start, this.props.start) ||
!isEqual(prevProps.end, this.props.end) ||
!isEqual(prevProps.period, this.props.period) ||
!isEqual(prevProps.query, this.props.query) ||
(!prevProps.enabled && this.props.enabled)) &&
enabled
) {
this.fetchData();
} else if (!isEqual(prevProps.emphasizeReleases, this.props.emphasizeReleases)) {
Expand Down

0 comments on commit ac0c4ac

Please sign in to comment.