From 9b79b287e92adbc5ec74e9281f86b1bd38959802 Mon Sep 17 00:00:00 2001 From: Ahmet Yesil Date: Wed, 7 Apr 2021 17:06:26 +0200 Subject: [PATCH] fix(ajax): bug fixes and readability improvements --- .changeset/eight-pumpkins-clean.md | 5 ++ packages/ajax/src/AjaxClient.js | 49 +++++------ packages/ajax/src/interceptors-cache.js | 85 ++++++------------- packages/ajax/src/interceptors.js | 7 +- packages/ajax/test/AjaxClient.test.js | 68 +++++++++++++-- packages/ajax/test/interceptors-cache.test.js | 2 +- 6 files changed, 116 insertions(+), 100 deletions(-) create mode 100644 .changeset/eight-pumpkins-clean.md diff --git a/.changeset/eight-pumpkins-clean.md b/.changeset/eight-pumpkins-clean.md new file mode 100644 index 000000000..a197c7a06 --- /dev/null +++ b/.changeset/eight-pumpkins-clean.md @@ -0,0 +1,5 @@ +--- +'@lion/ajax': patch +--- + +Fix(ajax) options expansion, fix removing request interceptors, use 1 hour as default time to live, check for null when serializing the search params diff --git a/packages/ajax/src/AjaxClient.js b/packages/ajax/src/AjaxClient.js index 996f02107..0c2481cd7 100644 --- a/packages/ajax/src/AjaxClient.js +++ b/packages/ajax/src/AjaxClient.js @@ -27,11 +27,11 @@ export class AjaxClient { xsrfCookieName: 'XSRF-TOKEN', xsrfHeaderName: 'X-XSRF-TOKEN', jsonPrefix: '', + ...config, cacheOptions: { getCacheIdentifier: () => '_default', ...config.cacheOptions, }, - ...config, }; /** @type {Array.} */ @@ -43,24 +43,18 @@ export class AjaxClient { this.addRequestInterceptor(acceptLanguageRequestInterceptor); } - if (this.__config.xsrfCookieName && this.__config.xsrfHeaderName) { - this.addRequestInterceptor( - createXSRFRequestInterceptor(this.__config.xsrfCookieName, this.__config.xsrfHeaderName), - ); + const { xsrfCookieName, xsrfHeaderName } = this.__config; + if (xsrfCookieName && xsrfHeaderName) { + this.addRequestInterceptor(createXSRFRequestInterceptor(xsrfCookieName, xsrfHeaderName)); } - if (this.__config.cacheOptions && this.__config.cacheOptions.useCache) { + const { cacheOptions } = this.__config; + if (cacheOptions?.useCache) { this.addRequestInterceptor( - cacheRequestInterceptorFactory( - this.__config.cacheOptions.getCacheIdentifier, - this.__config.cacheOptions, - ), + cacheRequestInterceptorFactory(cacheOptions.getCacheIdentifier, cacheOptions), ); this.addResponseInterceptor( - cacheResponseInterceptorFactory( - this.__config.cacheOptions.getCacheIdentifier, - this.__config.cacheOptions, - ), + cacheResponseInterceptorFactory(cacheOptions.getCacheIdentifier, cacheOptions), ); } } @@ -84,10 +78,9 @@ export class AjaxClient { /** @param {RequestInterceptor} requestInterceptor */ removeRequestInterceptor(requestInterceptor) { - const indexOf = this._requestInterceptors.indexOf(requestInterceptor); - if (indexOf !== -1) { - this._requestInterceptors.splice(indexOf); - } + this._requestInterceptors = this._requestInterceptors.filter( + interceptor => interceptor !== requestInterceptor, + ); } /** @param {ResponseInterceptor} responseInterceptor */ @@ -97,10 +90,9 @@ export class AjaxClient { /** @param {ResponseInterceptor} responseInterceptor */ removeResponseInterceptor(responseInterceptor) { - const indexOf = this._responseInterceptors.indexOf(responseInterceptor); - if (indexOf !== -1) { - this._responseInterceptors.splice(indexOf, 1); - } + this._responseInterceptors = this._responseInterceptors.filter( + interceptor => interceptor !== responseInterceptor, + ); } /** @@ -117,9 +109,9 @@ export class AjaxClient { request.params = init?.params; // run request interceptors, returning directly and skipping the network - // if a interceptor returns a Response const interceptedRequestOrResponse = await this.__interceptRequest(request); if (interceptedRequestOrResponse instanceof Response) { + // prevent network request, return cached response return interceptedRequestOrResponse; } @@ -147,12 +139,12 @@ export class AjaxClient { const lionInit = { ...init, headers: { - ...(init && init.headers), + ...init?.headers, accept: 'application/json', }, }; - if (lionInit && lionInit.body) { + if (lionInit?.body) { // eslint-disable-next-line no-param-reassign lionInit.headers['content-type'] = 'application/json'; lionInit.body = JSON.stringify(lionInit.body); @@ -163,10 +155,9 @@ export class AjaxClient { const response = await this.request(info, jsonInit); let responseText = await response.text(); - if (typeof this.__config.jsonPrefix === 'string') { - if (responseText.startsWith(this.__config.jsonPrefix)) { - responseText = responseText.substring(this.__config.jsonPrefix.length); - } + const { jsonPrefix } = this.__config; + if (typeof jsonPrefix === 'string' && responseText.startsWith(jsonPrefix)) { + responseText = responseText.substring(jsonPrefix.length); } try { diff --git a/packages/ajax/src/interceptors-cache.js b/packages/ajax/src/interceptors-cache.js index 5ad542a9a..750366736 100644 --- a/packages/ajax/src/interceptors-cache.js +++ b/packages/ajax/src/interceptors-cache.js @@ -6,15 +6,11 @@ import './typedef.js'; const SECOND = 1000; const MINUTE = SECOND * 60; const HOUR = MINUTE * 60; +const DEFAULT_TIME_TO_LIVE = HOUR; class Cache { constructor() { - this.expiration = new Date().getTime() + HOUR; - /** - * @type {{[url: string]: CacheConfig }} - */ - this.cacheConfig = {}; - + this.expiration = new Date().getTime() + DEFAULT_TIME_TO_LIVE; /** * @type {{[url: string]: {expires: number, response: CacheResponse} }} * @private @@ -97,7 +93,7 @@ class Cache { this._validateCache(); Object.keys(this._cacheObject).forEach(key => { - if (key.indexOf(url) > -1) { + if (key.includes(url)) { delete this._cacheObject[key]; this.resolvePendingRequest(key); } @@ -113,15 +109,9 @@ class Cache { Object.keys(this._cacheObject).forEach(key => { const notMatch = !new RegExp(regex).test(key); - if (notMatch) return; - - const isDataDeleted = delete this._cacheObject[key]; + delete this._cacheObject[key]; this.resolvePendingRequest(key); - - if (!isDataDeleted) { - throw new Error(`Failed to delete cache for a request '${key}'`); - } }); } @@ -135,6 +125,7 @@ class Cache { if (new Date().getTime() > this.expiration) { // @ts-ignore this._cacheObject = {}; + return false; } return true; } @@ -150,7 +141,7 @@ let caches = {}; */ export const searchParamSerializer = (params = {}) => // @ts-ignore - typeof params === 'object' ? new URLSearchParams(params).toString() : ''; + typeof params === 'object' && params !== null ? new URLSearchParams(params).toString() : ''; /** * Returns the active cache instance for the current session @@ -159,10 +150,9 @@ export const searchParamSerializer = (params = {}) => * @param {string} cacheIdentifier usually the refreshToken of the owner of the cache */ const getCache = cacheIdentifier => { - if (caches[cacheIdentifier] && caches[cacheIdentifier]._validateCache()) { + if (caches[cacheIdentifier]?._validateCache()) { return caches[cacheIdentifier]; } - // invalidate old caches caches = {}; // create new cache @@ -193,7 +183,7 @@ export const validateOptions = ({ // validate 'timeToLive', default 1 hour if (timeToLive === undefined) { - timeToLive = 0; + timeToLive = DEFAULT_TIME_TO_LIVE; } if (Number.isNaN(parseInt(String(timeToLive), 10))) { throw new Error('Property `timeToLive` must be of type `number`'); @@ -213,7 +203,7 @@ export const validateOptions = ({ // validate 'requestIdentificationFn', default is url + searchParams if (requestIdentificationFn) { if (typeof requestIdentificationFn !== 'function') { - throw new Error('Property `requestIdentificationFn` must be of type `function` or `falsy`'); + throw new Error('Property `requestIdentificationFn` must be of type `function`'); } } else { requestIdentificationFn = /** @param {any} data */ ( @@ -235,25 +225,6 @@ export const validateOptions = ({ }; }; -/** - * Request interceptor to return relevant cached requests - * @param {ValidatedCacheOptions} validatedInitialCacheOptions - * @param {CacheOptions=} configCacheOptions - * @returns {ValidatedCacheOptions} - */ -function composeCacheOptions(validatedInitialCacheOptions, configCacheOptions) { - let actionCacheOptions = validatedInitialCacheOptions; - - if (configCacheOptions) { - actionCacheOptions = validateOptions({ - ...validatedInitialCacheOptions, - ...configCacheOptions, - }); - } - - return actionCacheOptions; -} - /** * Request interceptor to return relevant cached requests * @param {function(): string} getCacheIdentifier used to invalidate cache if identifier is changed @@ -264,12 +235,11 @@ export const cacheRequestInterceptorFactory = (getCacheIdentifier, globalCacheOp const validatedInitialCacheOptions = validateOptions(globalCacheOptions); return /** @param {CacheRequest} cacheRequest */ async cacheRequest => { - const { method } = cacheRequest; + const cacheOptions = validateOptions({ + ...validatedInitialCacheOptions, + ...cacheRequest.cacheOptions, + }); - const cacheOptions = composeCacheOptions( - validatedInitialCacheOptions, - cacheRequest.cacheOptions, - ); cacheRequest.cacheOptions = cacheOptions; // don't use cache if 'useCache' === false @@ -278,12 +248,12 @@ export const cacheRequestInterceptorFactory = (getCacheIdentifier, globalCacheOp } const cacheId = cacheOptions.requestIdentificationFn(cacheRequest, searchParamSerializer); - // cacheIdentifier is used to bind the cache to the current session const currentCache = getCache(getCacheIdentifier()); + const { method } = cacheRequest; // don't use cache if the request method is not part of the configs methods - if (cacheOptions.methods.indexOf(method.toLowerCase()) === -1) { + if (!cacheOptions.methods.includes(method.toLowerCase())) { // If it's NOT one of the config.methods, invalidate caches currentCache.delete(cacheId); // also invalidate caches matching to cacheOptions @@ -310,11 +280,7 @@ export const cacheRequestInterceptorFactory = (getCacheIdentifier, globalCacheOp const cacheResponse = currentCache.get(cacheId, cacheOptions.timeToLive); if (cacheResponse) { - // eslint-disable-next-line no-param-reassign - if (!cacheRequest.cacheOptions) { - cacheRequest.cacheOptions = { useCache: false }; - } - + cacheRequest.cacheOptions = cacheRequest.cacheOptions ?? { useCache: false }; const response = /** @type {CacheResponse} */ cacheResponse.clone(); response.request = cacheRequest; response.fromCache = true; @@ -350,10 +316,11 @@ export const cacheResponseInterceptorFactory = (getCacheIdentifier, globalCacheO throw new Error('Missing request in response.'); } - const cacheOptions = composeCacheOptions( - validatedInitialCacheOptions, - cacheResponse.request?.cacheOptions, - ); + const cacheOptions = validateOptions({ + ...validatedInitialCacheOptions, + ...cacheResponse.request?.cacheOptions, + }); + // string that identifies cache entry const cacheId = cacheOptions.requestIdentificationFn( cacheResponse.request, @@ -363,13 +330,11 @@ export const cacheResponseInterceptorFactory = (getCacheIdentifier, globalCacheO const isAlreadyFromCache = !!cacheResponse.fromCache; // caching all responses with not default `timeToLive` const isCacheActive = cacheOptions.timeToLive > 0; - + const isMethodSupported = cacheOptions.methods.includes( + cacheResponse.request.method.toLowerCase(), + ); // if the request is one of the options.methods; store response in cache - if ( - !isAlreadyFromCache && - isCacheActive && - cacheOptions.methods.indexOf(cacheResponse.request.method.toLowerCase()) > -1 - ) { + if (!isAlreadyFromCache && isCacheActive && isMethodSupported) { // store the response data in the cache and mark request as resolved currentCache.set(cacheId, cacheResponse.clone()); } diff --git a/packages/ajax/src/interceptors.js b/packages/ajax/src/interceptors.js index 7a45b0fd3..41a6dd410 100644 --- a/packages/ajax/src/interceptors.js +++ b/packages/ajax/src/interceptors.js @@ -17,10 +17,9 @@ export function getCookie(name, _document = document) { */ export async function acceptLanguageRequestInterceptor(request) { if (!request.headers.has('accept-language')) { - let locale = document.documentElement.lang || 'en'; - if (document.documentElement.getAttribute('data-localize-lang')) { - locale = document.documentElement.getAttribute('data-localize-lang') || 'en'; - } + const documentLocale = document.documentElement.lang; + const localizeLang = document.documentElement.getAttribute('data-localize-lang'); + const locale = localizeLang || documentLocale || 'en'; request.headers.set('accept-language', locale); } return request; diff --git a/packages/ajax/test/AjaxClient.test.js b/packages/ajax/test/AjaxClient.test.js index 9c36556db..9947feefb 100644 --- a/packages/ajax/test/AjaxClient.test.js +++ b/packages/ajax/test/AjaxClient.test.js @@ -18,6 +18,54 @@ describe('AjaxClient', () => { fetchStub.restore(); }); + describe('options', () => { + it('creates options object by expanding cacheOptions', async () => { + // Given + const getCacheIdentifier = () => '_DEFAULT_CACHE_ID'; + const config = { + jsonPrefix: ")]}',", + cacheOptions: { + useCache: true, + timeToLive: 1000 * 60 * 5, // 5 minutes + getCacheIdentifier, + }, + }; + const expected = { + addAcceptLanguage: true, + xsrfCookieName: 'XSRF-TOKEN', + xsrfHeaderName: 'X-XSRF-TOKEN', + jsonPrefix: ")]}',", + cacheOptions: { + useCache: true, + timeToLive: 300000, + getCacheIdentifier, + }, + }; + // When + const ajax1 = new AjaxClient(config); + const result = ajax1.options; + // Then + expect(result).to.deep.equal(expected); + }); + + it('has default getCacheIdentifier function when cacheOptions does not provide one', async () => { + // Given + const config = { + cacheOptions: { + useCache: true, + timeToLive: 1000 * 60 * 5, // 5 minutes + }, + }; + // When + // @ts-expect-error + const ajax1 = new AjaxClient(config); + const result = ajax1.options?.cacheOptions?.getCacheIdentifier; + // Then + expect(result).not.to.be.undefined; + expect(result).to.be.a('function'); + }); + }); + describe('request()', () => { it('calls fetch with the given args, returning the result', async () => { const response = await (await ajax.request('/foo', { method: 'POST' })).text(); @@ -130,15 +178,22 @@ describe('AjaxClient', () => { }); it('removeRequestInterceptor() removes a request interceptor', async () => { - const interceptor = /** @param {Request} r */ async r => + const interceptor1 = /** @param {Request} r */ async r => new Request(`${r.url}/intercepted-1`); - ajax.addRequestInterceptor(interceptor); - ajax.removeRequestInterceptor(interceptor); + const interceptor2 = /** @param {Request} r */ async r => + new Request(`${r.url}/intercepted-2`); + const interceptor3 = /** @param {Request} r */ async r => + new Request(`${r.url}/intercepted-3`); + + ajax.addRequestInterceptor(interceptor1); + ajax.addRequestInterceptor(interceptor2); + ajax.addRequestInterceptor(interceptor3); + ajax.removeRequestInterceptor(interceptor1); await ajax.request('/foo', { method: 'POST' }); const request = fetchStub.getCall(0).args[0]; - expect(request.url).to.equal(`${window.location.origin}/foo`); + expect(request.url).to.equal(`${window.location.origin}/foo/intercepted-2/intercepted-3`); }); it('removeResponseInterceptor() removes a request interceptor', async () => { @@ -148,8 +203,9 @@ describe('AjaxClient', () => { // @ts-expect-error we're mocking the response as a simple promise which returns a string ajax.removeResponseInterceptor(interceptor); - const response = await (await ajax.request('/foo', { method: 'POST' })).text(); - expect(response).to.equal('mock response'); + const response = await ajax.request('/foo', { method: 'POST' }); + const text = await response.text(); + expect(text).to.equal('mock response'); }); }); diff --git a/packages/ajax/test/interceptors-cache.test.js b/packages/ajax/test/interceptors-cache.test.js index b0bf7866e..35a738dbd 100644 --- a/packages/ajax/test/interceptors-cache.test.js +++ b/packages/ajax/test/interceptors-cache.test.js @@ -135,7 +135,7 @@ describe('ajax cache', () => { requestIdentificationFn: 'not a function', }); removeCacheInterceptors(ajax, indexes); - }).to.throw(/Property `requestIdentificationFn` must be of type `function` or `falsy`/); + }).to.throw(/Property `requestIdentificationFn` must be of type `function`/); }); });