Skip to content

Commit 15f93d2

Browse files
author
Juarez Mota
committed
fix: revert message picker changes
1 parent 98eedb6 commit 15f93d2

File tree

4 files changed

+85
-210
lines changed

4 files changed

+85
-210
lines changed

‎dotcom-rendering/src/components/StickyBottomBanner.importable.tsx‎

Lines changed: 6 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -76,36 +76,12 @@ const DEFAULT_BANNER_TIMEOUT_MILLIS = 2000;
7676
const buildCmpBannerConfig = (): CandidateConfig<void> => ({
7777
candidate: {
7878
id: 'cmpUi',
79-
canShow: () => {
80-
// In Storybook environment, CMP APIs may not be available
81-
// Return false immediately to avoid hanging
82-
if (
83-
typeof window !== 'undefined' &&
84-
(window.location.hostname === 'localhost' ||
85-
window.location.hostname === '127.0.0.1')
86-
) {
87-
// eslint-disable-next-line no-console -- Required for debugging CMP issues
88-
console.log(
89-
'CMP: Detected Storybook/localhost environment, skipping CMP check',
90-
);
91-
return Promise.resolve({ show: false as const });
92-
}
93-
94-
try {
95-
return cmp
96-
.willShowPrivacyMessage()
97-
.then((result) => {
98-
return result
99-
? { show: true as const, meta: undefined }
100-
: { show: false as const };
101-
})
102-
.catch(() => {
103-
return { show: false as const };
104-
});
105-
} catch (error) {
106-
return Promise.resolve({ show: false as const });
107-
}
108-
},
79+
canShow: () =>
80+
cmp
81+
.willShowPrivacyMessage()
82+
.then((result) =>
83+
result ? { show: true, meta: undefined } : { show: false },
84+
), // Intentionally there is no catch block because if the CMP fails no other banner can be displayed.
10985
show: () => {
11086
// New CMP is not a react component and is shown outside of react's world
11187
// so render nothing if it will show

‎dotcom-rendering/src/components/StickyBottomBanner.stories.tsx‎

Lines changed: 0 additions & 74 deletions
This file was deleted.

‎dotcom-rendering/src/lib/messagePicker.test.tsx‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// using Promise.resolve for flushing microtasks works with fake timers
1+
import { setImmediate } from 'node:timers';
22
import type { CanShowResult, SlotConfig } from './messagePicker';
33
import { pickMessage } from './messagePicker';
44

@@ -11,10 +11,10 @@ jest.useFakeTimers();
1111

1212
// Wait for all unsettled promises to complete before finishing the test. Not
1313
// doing this results in a warning from Jest. Note that it's actually expected
14-
// that there may be outstanding promises when a test completes because the
14+
// that there may be outstanding promises when a test completes becuase the the
1515
// message picker returns when the canShow of a message resolves, even if there
1616
// are lower priority messages still pending.
17-
const flushPromises = () => Promise.resolve();
17+
const flushPromises = () => new Promise(setImmediate);
1818
afterEach(async () => {
1919
await flushPromises();
2020
jest.clearAllMocks();
@@ -129,8 +129,6 @@ describe('pickMessage', () => {
129129

130130
const messagePromise = pickMessage(config, 'Web');
131131
jest.advanceTimersByTime(260);
132-
await flushPromises();
133-
jest.advanceTimersByTime(260);
134132
const got = await messagePromise;
135133

136134
expect(got()).toEqual(ChosenMockComponent);
@@ -184,8 +182,6 @@ describe('pickMessage', () => {
184182

185183
const messagePromise = pickMessage(config, 'Web');
186184
jest.advanceTimersByTime(260);
187-
await flushPromises();
188-
jest.advanceTimersByTime(260);
189185
const got = await messagePromise;
190186

191187
expect(got()).toEqual(null);

‎dotcom-rendering/src/lib/messagePicker.ts‎

Lines changed: 76 additions & 99 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { startPerformanceMeasure } from '@guardian/libs';
1+
import { isUndefined, startPerformanceMeasure } from '@guardian/libs';
22
import { getOphan } from '../client/ophan/ophan';
33
import type { RenderingTarget } from '../types/renderingTarget';
44

@@ -57,8 +57,8 @@ const timeoutify = <T>(
5757
): CandidateConfigWithTimeout<T> => {
5858
let timer: number | undefined;
5959

60-
const canShow = (): Promise<CanShowResult<T>> => {
61-
return new Promise((resolve) => {
60+
const canShow = (): Promise<CanShowResult<T>> =>
61+
new Promise((resolve) => {
6262
const perfName = `messagePicker-canShow-${candidateConfig.candidate.id}`;
6363
const { endPerformanceMeasure } = startPerformanceMeasure(
6464
'tx',
@@ -72,53 +72,34 @@ const timeoutify = <T>(
7272
slotName,
7373
renderingTarget,
7474
);
75-
cancelTimeout();
7675
resolve({ show: false });
7776
}, candidateConfig.timeoutMillis);
7877
}
7978

80-
try {
81-
candidateConfig.candidate
82-
.canShow()
83-
.then((result) => {
84-
resolve(result);
85-
86-
const canShowTimeTaken = endPerformanceMeasure();
87-
88-
if (candidateConfig.reportTiming) {
89-
void getOphan(renderingTarget).then((ophan) => {
90-
ophan.record({
91-
// @ts-expect-error -- the relevant team should remove this call as it is dropped by Ophan
92-
// see https://github.com/guardian/dotcom-rendering/pull/11438 further context
93-
component: perfName,
94-
value: canShowTimeTaken,
95-
});
79+
candidateConfig.candidate
80+
.canShow()
81+
.then((result) => {
82+
resolve(result);
83+
84+
const canShowTimeTaken = endPerformanceMeasure();
85+
86+
if (candidateConfig.reportTiming) {
87+
void getOphan(renderingTarget).then((ophan) => {
88+
ophan.record({
89+
// @ts-expect-error -- the relevant team should remove this call as it is dropped by Ophan
90+
// see https://github.com/guardian/dotcom-rendering/pull/11438 further context
91+
component: perfName,
92+
value: canShowTimeTaken,
9693
});
97-
}
98-
})
99-
.catch((e) => {
100-
console.error(
101-
`timeoutify candidate - error: ${String(e)}`,
102-
);
103-
resolve({ show: false });
104-
})
105-
.finally(() => {
106-
cancelTimeout();
107-
});
108-
} catch (error) {
109-
console.error(`timeoutify candidate - error: ${String(error)}`);
110-
cancelTimeout();
111-
resolve({ show: false });
112-
}
94+
});
95+
}
96+
})
97+
.catch((e) =>
98+
console.error(`timeoutify candidate - error: ${String(e)}`),
99+
);
113100
});
114-
};
115101

116-
const cancelTimeout = () => {
117-
if (timer !== undefined) {
118-
clearTimeout(timer);
119-
timer = undefined;
120-
}
121-
};
102+
const cancelTimeout = () => !isUndefined(timer) && clearTimeout(timer);
122103

123104
return {
124105
...candidateConfig,
@@ -130,71 +111,67 @@ const timeoutify = <T>(
130111
};
131112
};
132113

133-
const clearAllTimeouts = (
134-
candidateConfigs: CandidateConfigWithTimeout<any>[],
135-
) => {
136-
for (const config of candidateConfigs) {
137-
config.cancelTimeout();
138-
}
139-
};
114+
const clearAllTimeouts = (messages: CandidateConfigWithTimeout<any>[]) =>
115+
messages.map((m) => m.cancelTimeout());
140116

141117
const defaultShow = () => null;
142118

143-
/**
144-
* Sequential message picker that respects priority order.
145-
*
146-
* This processes candidates one by one in the order they appear in the array,
147-
* ensuring that higher priority messages are always checked first.
148-
*/
149-
export async function pickMessage(
119+
interface PendingMessage<T> {
120+
candidateConfig: CandidateConfigWithTimeout<T>;
121+
canShow: Promise<CanShowResult<T>>;
122+
}
123+
124+
interface WinningMessage<T> {
125+
meta: T;
126+
candidate: Candidate<T>;
127+
}
128+
129+
export const pickMessage = (
150130
{ candidates, name }: SlotConfig,
151131
renderingTarget: RenderingTarget,
152-
): Promise<() => MaybeFC> {
153-
const candidateConfigsWithTimeout = candidates.map((c) =>
154-
timeoutify(c, name, renderingTarget),
155-
);
156-
157-
try {
158-
const settled = await Promise.allSettled(
159-
candidateConfigsWithTimeout.map((config) =>
160-
config.candidate.canShow(),
161-
),
132+
): Promise<() => MaybeFC> =>
133+
new Promise((resolve) => {
134+
const candidateConfigsWithTimeout = candidates.map((c) =>
135+
timeoutify(c, name, renderingTarget),
136+
);
137+
const results: PendingMessage<any>[] = candidateConfigsWithTimeout.map(
138+
(candidateConfig) => ({
139+
candidateConfig,
140+
canShow: candidateConfig.candidate.canShow(),
141+
}),
162142
);
163143

164-
for (let i = 0; i < candidateConfigsWithTimeout.length; i++) {
165-
const config = candidateConfigsWithTimeout[i];
166-
const result = settled[i];
167-
168-
config?.cancelTimeout();
144+
const winnerResult = results.reduce<
145+
Promise<WinningMessage<any> | null>
146+
>(async (winningMessageSoFar, { candidateConfig, canShow }) => {
147+
if (await winningMessageSoFar) {
148+
return winningMessageSoFar;
149+
}
169150

170-
if (
171-
result?.status === 'rejected' &&
172-
result.reason instanceof Error
173-
) {
174-
window.guardian.modules.sentry.reportError(
175-
result.reason,
176-
`pickMessage: error checking ${config?.candidate.id}`,
177-
);
178-
continue;
151+
const result = await canShow;
152+
candidateConfig.cancelTimeout();
153+
if (result.show) {
154+
return {
155+
candidate: candidateConfig.candidate,
156+
meta: result.meta,
157+
};
179158
}
180159

181-
const canShowResult =
182-
result?.status === 'fulfilled' ? result.value : undefined;
183-
if (canShowResult?.show) {
160+
return winningMessageSoFar;
161+
}, Promise.resolve(null));
162+
163+
winnerResult
164+
.then((winner) => {
184165
clearAllTimeouts(candidateConfigsWithTimeout);
185-
return () => config?.candidate.show(canShowResult.meta) ?? null;
186-
}
187-
}
188-
189-
return defaultShow;
190-
} catch (fatal) {
191-
if (fatal instanceof Error) {
192-
window.guardian.modules.sentry.reportError(
193-
fatal,
194-
`pickMessage: fatal error`,
166+
167+
if (winner === null) {
168+
resolve(defaultShow);
169+
} else {
170+
const { candidate, meta } = winner;
171+
resolve(() => candidate.show(meta));
172+
}
173+
})
174+
.catch((e) =>
175+
console.error(`pickMessage winner - error: ${String(e)}`),
195176
);
196-
}
197-
clearAllTimeouts(candidateConfigsWithTimeout);
198-
return defaultShow;
199-
}
200-
}
177+
});

0 commit comments

Comments
 (0)