diff --git a/src/contentFetcher.ts b/src/contentFetcher.ts index 93334e38..cbba7fb7 100644 --- a/src/contentFetcher.ts +++ b/src/contentFetcher.ts @@ -128,15 +128,15 @@ export class ContentFetcher { throw new Error('The API key must be provided to fetch static content.'); } - // Types for Browser and Node environment - let timeoutReference: number | NodeJS.Timeout | undefined; return new Promise>((resolve, reject) => { const abortController = new AbortController(); const timeout = options.timeout ?? this.configuration.defaultTimeout; + let timer: number | NodeJS.Timeout | undefined; + if (timeout !== undefined) { - timeoutReference = setTimeout( + timer = setTimeout( () => { const response: ErrorResponse = { title: `Content could not be loaded in time for slot '${slotId}'.`, @@ -156,6 +156,7 @@ export class ContentFetcher { } this.load(slotId, abortController.signal, options) + .finally(() => clearTimeout(timer)) .then(response => { const region = response.headers.get('X-Croct-Region'); const timing = response.headers.get('X-Croct-Timing'); @@ -192,12 +193,7 @@ export class ContentFetcher { ); } }); - }) - .finally(() => { - // Once the fetch completes, regardless of outcome, cancel the timeout - // to avoid logging an error that didn't happen. - clearTimeout(timeoutReference); - }); + }); } private load(slotId: string, signal: AbortSignal, options: FetchOptions): Promise { diff --git a/src/evaluator.ts b/src/evaluator.ts index a980de55..c54ee8b3 100644 --- a/src/evaluator.ts +++ b/src/evaluator.ts @@ -167,6 +167,8 @@ export class Evaluator { const abortController = new AbortController(); const timeout = options.timeout ?? this.configuration.defaultTimeout; + let timer: number | NodeJS.Timeout | undefined; + if (timeout !== undefined) { setTimeout( () => { @@ -187,63 +189,64 @@ export class Evaluator { ); } - const promise = this.fetch(payload, abortController.signal, options); - - promise.then( - response => { - const region = response.headers.get('X-Croct-Region'); - const timing = response.headers.get('X-Croct-Timing'); - - this.logger.debug( - `Evaluation of the query "${reference}" processed by region ${region} in ${timing}.`, - ); - - return response.json() - .then(body => { - if (response.ok) { - return resolve(body); - } - - this.logHelp(response.status); - - const problem: ErrorResponse = body; - - switch (problem.type) { - case EvaluationErrorType.INVALID_QUERY: - case EvaluationErrorType.EVALUATION_FAILED: - case EvaluationErrorType.TOO_COMPLEX_QUERY: - reject(new QueryError(problem as QueryErrorResponse)); - - break; - - default: - reject(new EvaluationError(problem)); - - break; - } - }) - .catch(error => { - if (!response.ok) { - throw new Error(`Error ${response.status} - ${response.statusText}`); - } - - throw error; - }); - }, - ).catch( - error => { - if (!abortController.signal.aborted) { - reject( - new EvaluationError({ - title: formatMessage(error), - type: EvaluationErrorType.UNEXPECTED_ERROR, - detail: 'Please try again or contact Croct support if the error persists.', - status: 500, // Internal Server Error - }), + this.fetch(payload, abortController.signal, options) + .finally(() => clearTimeout(timer)) + .then( + response => { + const region = response.headers.get('X-Croct-Region'); + const timing = response.headers.get('X-Croct-Timing'); + + this.logger.debug( + `Evaluation of the query "${reference}" processed by region ${region} in ${timing}.`, ); - } - }, - ); + + return response.json() + .then(body => { + if (response.ok) { + return resolve(body); + } + + this.logHelp(response.status); + + const problem: ErrorResponse = body; + + switch (problem.type) { + case EvaluationErrorType.INVALID_QUERY: + case EvaluationErrorType.EVALUATION_FAILED: + case EvaluationErrorType.TOO_COMPLEX_QUERY: + reject(new QueryError(problem as QueryErrorResponse)); + + break; + + default: + reject(new EvaluationError(problem)); + + break; + } + }) + .catch(error => { + if (!response.ok) { + throw new Error(`Error ${response.status} - ${response.statusText}`); + } + + throw error; + }); + }, + ) + .catch( + error => { + if (!abortController.signal.aborted) { + reject( + new EvaluationError({ + title: formatMessage(error), + type: EvaluationErrorType.UNEXPECTED_ERROR, + detail: 'Please try again or contact Croct support if the error persists.', + status: 500, // Internal Server Error + }), + ); + } + }, + ); }); } diff --git a/test/evaluator.test.ts b/test/evaluator.test.ts index 7c333623..7d036fa1 100644 --- a/test/evaluator.test.ts +++ b/test/evaluator.test.ts @@ -277,9 +277,7 @@ describe('An evaluator', () => { fetchMock.mock({ ...requestMatcher, delay: 20, - response: { - result: 'Carol', - }, + response: JSON.stringify('Carol'), }); const promise = evaluator.evaluate(query, { @@ -315,9 +313,7 @@ describe('An evaluator', () => { fetchMock.mock({ ...requestMatcher, delay: 20, - response: { - result: 'Carol', - }, + response: JSON.stringify('Carol'), }); const promise = evaluator.evaluate(query); @@ -333,6 +329,31 @@ describe('An evaluator', () => { }); }); + it('should not log a timeout error message when request completes before the timeout', async () => { + jest.useFakeTimers(); + + const logger: Logger = { + debug: jest.fn(), + info: jest.fn(), + warn: jest.fn(), + error: jest.fn(), + }; + + const evaluator = new Evaluator({ + appId: appId, + logger: logger, + }); + + fetchMock.mock({ + ...requestMatcher, + response: JSON.stringify('Carol'), + }); + + await expect(evaluator.evaluate(query, {timeout: 10})).resolves.toBe('Carol'); + + expect(logger.error).not.toHaveBeenCalled(); + }); + it('should evaluate queries using the provided context', async () => { const evaluator = new Evaluator({ appId: appId,