Skip to content

Commit 7e60b62

Browse files
authored
ref: Make includeAllArgs mandatory in doEventsRequest() and iterate on types (#87682)
`doEventsRequest()` previously allowed the prop `includeAllArgs` to be optional, and was typed based on whether that field was set to `true` or `false | undefined`. Now the field is required, no longer optional. I also made sure to be explicit about setting the generic, so TS doesn't have to infer it, in order to make all the callsites as explicit as possible. So you don't need to look at `props` or something to figure out what the return type will be. The async test stuff seemed to improve some flakeyness I was seeing too.
1 parent 3eb7768 commit 7e60b62

File tree

11 files changed

+33
-24
lines changed

11 files changed

+33
-24
lines changed

static/app/actionCreators/events.spec.tsx

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -32,9 +32,10 @@ describe('Events ActionCreator', function () {
3232
});
3333
});
3434

35-
it('requests events stats with relative period', function () {
36-
doEventsRequest(api, {
35+
it('requests events stats with relative period', async function () {
36+
await doEventsRequest<false>(api, {
3737
...opts,
38+
includeAllArgs: false,
3839
includePrevious: false,
3940
period: '7d',
4041
partial: true,
@@ -52,9 +53,10 @@ describe('Events ActionCreator', function () {
5253
);
5354
});
5455

55-
it('sets useRpc param', function () {
56-
doEventsRequest(api, {
56+
it('sets useRpc param', async function () {
57+
await doEventsRequest<false>(api, {
5758
...opts,
59+
includeAllArgs: false,
5860
includePrevious: false,
5961
period: '7d',
6062
partial: true,
@@ -73,9 +75,10 @@ describe('Events ActionCreator', function () {
7375
);
7476
});
7577

76-
it('requests events stats with relative period including previous period', function () {
77-
doEventsRequest(api, {
78+
it('requests events stats with relative period including previous period', async function () {
79+
await doEventsRequest<false>(api, {
7880
...opts,
81+
includeAllArgs: false,
7982
includePrevious: true,
8083
period: '7d',
8184
partial: true,
@@ -93,11 +96,12 @@ describe('Events ActionCreator', function () {
9396
);
9497
});
9598

96-
it('requests events stats with absolute period', function () {
99+
it('requests events stats with absolute period', async function () {
97100
const start = new Date('2017-10-12T12:00:00.000Z');
98101
const end = new Date('2017-10-17T00:00:00.000Z');
99-
doEventsRequest(api, {
102+
await doEventsRequest<false>(api, {
100103
...opts,
104+
includeAllArgs: false,
101105
includePrevious: false,
102106
start,
103107
end,
@@ -121,8 +125,9 @@ describe('Events ActionCreator', function () {
121125
it('requests events stats with absolute period including previous period', async function () {
122126
const start = new Date('2017-10-12T12:00:00.000Z');
123127
const end = new Date('2017-10-17T00:00:00.000Z');
124-
await doEventsRequest(api, {
128+
await doEventsRequest<false>(api, {
125129
...opts,
130+
includeAllArgs: false,
126131
includePrevious: true,
127132
start,
128133
end,
@@ -143,8 +148,9 @@ describe('Events ActionCreator', function () {
143148
});
144149

145150
it('spreads query extras', async function () {
146-
await doEventsRequest(api, {
151+
await doEventsRequest<false>(api, {
147152
...opts,
153+
includeAllArgs: false,
148154
queryExtras: {useOnDemandMetrics: 'true'},
149155
partial: true,
150156
});

static/app/actionCreators/events.tsx

Lines changed: 4 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@ import type {LocationDescriptor} from 'history';
22
import pick from 'lodash/pick';
33

44
import {addErrorMessage} from 'sentry/actionCreators/indicator';
5-
import type {ApiResult, Client, ResponseMeta} from 'sentry/api';
5+
import type {ApiResult, Client} from 'sentry/api';
66
import {canIncludePreviousPeriod} from 'sentry/components/charts/utils';
77
import {t} from 'sentry/locale';
88
import type {DateString} from 'sentry/types/core';
@@ -61,7 +61,7 @@ type Options = {
6161
yAxis?: string | string[];
6262
};
6363

64-
export type EventsStatsOptions<T extends boolean> = {includeAllArgs?: T} & Options;
64+
export type EventsStatsOptions<T extends boolean> = {includeAllArgs: T} & Options;
6565

6666
/**
6767
* Make requests to `events-stats` endpoint
@@ -83,7 +83,7 @@ export type EventsStatsOptions<T extends boolean> = {includeAllArgs?: T} & Optio
8383
* @param {Record<string, string>} options.queryExtras A list of extra query parameters
8484
* @param {(org: OrganizationSummary) => string} options.generatePathname A function that returns an override for the pathname
8585
*/
86-
export const doEventsRequest = <IncludeAllArgsType extends boolean = false>(
86+
export const doEventsRequest = <IncludeAllArgsType extends boolean>(
8787
api: Client,
8888
{
8989
organization,
@@ -113,9 +113,7 @@ export const doEventsRequest = <IncludeAllArgsType extends boolean = false>(
113113
useRpc,
114114
}: EventsStatsOptions<IncludeAllArgsType>
115115
): IncludeAllArgsType extends true
116-
? Promise<
117-
[EventsStats | MultiSeriesEventsStats, string | undefined, ResponseMeta | undefined]
118-
>
116+
? Promise<ApiResult<EventsStats | MultiSeriesEventsStats>>
119117
: Promise<EventsStats | MultiSeriesEventsStats> => {
120118
const pathname =
121119
generatePathname?.(organization) ??

static/app/components/charts/eventsRequest.spec.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ describe('EventsRequest', function () {
2828
query: '',
2929
children: () => null,
3030
partial: false,
31+
includeAllArgs: false,
3132
includeTransformedData: true,
3233
};
3334

static/app/components/charts/eventsRequest.tsx

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -65,6 +65,7 @@ export type RenderProps = LoadingStatus &
6565
};
6666

6767
type DefaultProps = {
68+
includeAllArgs: false;
6869
/**
6970
* Include data for previous period
7071
*/
@@ -277,6 +278,7 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
277278
comparisonDelta: undefined,
278279
limit: 15,
279280
query: '',
281+
includeAllArgs: false,
280282
includePrevious: true,
281283
includeTransformedData: true,
282284
};
@@ -334,7 +336,7 @@ class EventsRequest extends PureComponent<EventsRequestProps, EventsRequestState
334336
} else {
335337
try {
336338
api.clear();
337-
timeseriesData = await doEventsRequest(api, props);
339+
timeseriesData = await doEventsRequest<false>(api, props);
338340
} catch (resp) {
339341
if (resp?.responseJSON?.detail) {
340342
errorMessage = resp.responseJSON.detail;

static/app/components/charts/onDemandMetricRequest.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import type {EventsStats, MultiSeriesEventsStats} from 'sentry/types/organizatio
77
export class OnDemandMetricRequest extends EventsRequest {
88
fetchExtrapolatedData = async (): Promise<EventsStats> => {
99
const {api, organization, ...props} = this.props;
10-
const retVal = await doEventsRequest(api, {
10+
const retVal = await doEventsRequest<false>(api, {
1111
...props,
1212
organization,
1313
generatePathname: () =>

static/app/views/alerts/rules/metric/triggers/chart/index.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -571,6 +571,7 @@ class TriggersChart extends PureComponent<Props, State> {
571571
if (isOnDemandMetricAlert) {
572572
const {sampleRate} = this.state;
573573
const baseProps: EventsRequestProps = {
574+
includeAllArgs: false,
574575
api,
575576
organization,
576577
query,

static/app/views/dashboards/datasetConfig/errorsAndTransactions.tsx

Lines changed: 3 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -634,8 +634,9 @@ export async function doOnDemandMetricsRequest(
634634
const fetchEstimatedStats = () =>
635635
`/organizations/${requestData.organization.slug}/metrics-estimation-stats/`;
636636

637-
const response = await doEventsRequest<false>(api, {
637+
const response = await doEventsRequest<true>(api, {
638638
...requestData,
639+
includeAllArgs: true,
639640
queryExtras: {
640641
...requestData.queryExtras,
641642
useOnDemandMetrics: true,
@@ -645,21 +646,17 @@ export async function doOnDemandMetricsRequest(
645646
generatePathname: isEditing ? fetchEstimatedStats : undefined,
646647
});
647648

648-
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
649649
response[0] = {...response[0]};
650650

651651
if (
652652
hasDatasetSelector(requestData.organization) &&
653653
widgetType === WidgetType.DISCOVER
654654
) {
655-
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
656-
const meta = response[0].meta ?? {};
655+
const meta: any = response[0].meta ?? {};
657656
meta.discoverSplitDecision = 'transaction-like';
658-
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
659657
response[0] = {...response[0], ...{meta}};
660658
}
661659

662-
// @ts-expect-error TS(7053): Element implicitly has an 'any' type because expre... Remove this comment to see the full error message
663660
return [response[0], response[1], response[2]];
664661
} catch (err) {
665662
Sentry.captureMessage('Failed to fetch metrics estimation stats', {extra: err});

static/app/views/performance/landing/widgets/widgets/lineChartListWidget.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -472,6 +472,7 @@ export function LineChartListWidget(props: PerformanceWidgetProps) {
472472
return (
473473
<EventsRequest
474474
{...pick(provided, eventsRequestQueryProps)}
475+
includeAllArgs={false}
475476
yAxis={yAxis}
476477
limit={1}
477478
includePrevious={includePreviousParam}

static/app/views/performance/landing/widgets/widgets/singleFieldAreaWidget.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ export function SingleFieldAreaWidget(props: PerformanceWidgetProps) {
4747
{({queryBatching}) => (
4848
<EventsRequest
4949
{...pick(provided, eventsRequestQueryProps)}
50+
includeAllArgs={false}
5051
limit={1}
5152
queryBatching={queryBatching}
5253
includePrevious

static/app/views/performance/landing/widgets/widgets/stackedAreaChartListWidget.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,6 +154,7 @@ export function StackedAreaChartListWidget(props: PerformanceWidgetProps) {
154154
return (
155155
<EventsRequest
156156
{...pick(prunedProvided, eventsRequestQueryProps)}
157+
includeAllArgs={false}
157158
limit={5}
158159
includePrevious={false}
159160
includeTransformedData

static/app/views/performance/landing/widgets/widgets/vitalWidget.tsx

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,7 @@ export function VitalWidget(props: PerformanceWidgetProps) {
202202
return (
203203
<EventsRequest
204204
{...requestProps}
205+
includeAllArgs={false}
205206
limit={1}
206207
currentSeriesNames={[sortField!]}
207208
includePrevious={false}

0 commit comments

Comments
 (0)