From a7a4e31e573a50fe38309809acd037fb3448040b Mon Sep 17 00:00:00 2001 From: Arthur Fiorette <47537704+arthurfiorette@users.noreply.github.com> Date: Fri, 18 Oct 2024 11:09:15 -0300 Subject: [PATCH] fix: cloneData for concurrent requests (#921) * code * code * reuse my biome config * fix breaking change on 1.6.0 * lint --- biome.json | 27 +----------------- docs/src/config.md | 16 +++++++---- docs/src/config/request-specifics.md | 2 +- docs/src/guide/storages.md | 10 +++++-- package.json | 13 +++++---- pnpm-lock.yaml | 8 ++++++ src/cache/cache.ts | 7 ++--- src/cache/create.ts | 2 +- src/interceptors/request.ts | 35 ++++++++++++++++++++---- src/interceptors/response.ts | 37 +++++++++++++++++-------- src/storage/build.ts | 7 ----- src/storage/memory.ts | 40 ++++++++++----------------- src/storage/types.ts | 7 +++-- test/interceptors/request.test.ts | 41 ++++++++++++++++++++++++++-- test/mocks/axios.ts | 7 +++-- 15 files changed, 154 insertions(+), 105 deletions(-) diff --git a/biome.json b/biome.json index 0b8114ee..d6313b39 100644 --- a/biome.json +++ b/biome.json @@ -1,31 +1,6 @@ { "$schema": "https://biomejs.dev/schemas/1.9.0/schema.json", - "organizeImports": { - "enabled": true - }, - "linter": { - "enabled": true, - "rules": { - "recommended": true, - "style": { - "noNonNullAssertion": "off", - "noParameterAssign": "off" - }, - "suspicious": { - "noExplicitAny": "off" - } - } - }, - "formatter": { - "lineWidth": 100, - "indentStyle": "space" - }, - "javascript": { - "formatter": { - "quoteStyle": "single", - "trailingCommas": "none" - } - }, + "extends": ["@arthurfiorette/biomejs-config"], "files": { "ignore": [ "build/**/*", diff --git a/docs/src/config.md b/docs/src/config.md index 7e35ea67..1ad2220c 100644 --- a/docs/src/config.md +++ b/docs/src/config.md @@ -62,14 +62,14 @@ In any persistent cache scenario where hitting over 77K unique keys is a possibi -- Type: `Record>` -- Default: `{}` +- Type: `Map>` +- Default: `new Map` A simple object that will hold a promise for each pending request. Used to handle concurrent requests. -You'd normally not need to change this, but it is exposed in case you need to use it as -some sort of listener of know when a request is waiting for other to finish. +You shouldn't change this property, but it is exposed in case you need to use it as some +sort of listener or know when a request is waiting for others to finish. ## headerInterpreter @@ -102,7 +102,10 @@ The possible returns are: ::: details Example of a custom headerInterpreter ```ts -import { setupCache, type HeaderInterpreter } from 'axios-cache-interceptor'; +import { + setupCache, + type HeaderInterpreter +} from 'axios-cache-interceptor'; const myHeaderInterpreter: HeaderInterpreter = (headers) => { if (headers['x-my-custom-header']) { @@ -186,7 +189,8 @@ setupCache(axiosInstance, { debug: console.log }); // Own logging platform. setupCache(axiosInstance, { - debug: ({ id, msg, data }) => myLoggerExample.emit({ id, msg, data }) + debug: ({ id, msg, data }) => + myLoggerExample.emit({ id, msg, data }) }); // Disables debug. (default) diff --git a/docs/src/config/request-specifics.md b/docs/src/config/request-specifics.md index eeb774c8..651c64d8 100644 --- a/docs/src/config/request-specifics.md +++ b/docs/src/config/request-specifics.md @@ -148,7 +148,7 @@ and in this [StackOverflow](https://stackoverflow.com/a/62781874/14681561) answe - Type: `Method[]` -- Default: `["get"]` +- Default: `["get", "head"]` Specifies which methods we should handle and cache. This is where you can enable caching to `POST`, `PUT`, `DELETE` and other methods, as the default is only `GET`. diff --git a/docs/src/guide/storages.md b/docs/src/guide/storages.md index 03667205..15a059d9 100644 --- a/docs/src/guide/storages.md +++ b/docs/src/guide/storages.md @@ -38,7 +38,10 @@ For long running processes, you can avoid memory leaks by using playing with the ```ts import Axios from 'axios'; -import { setupCache, buildMemoryStorage } from 'axios-cache-interceptor'; +import { + setupCache, + buildMemoryStorage +} from 'axios-cache-interceptor'; setupCache(axios, { // You don't need to to that, as it is the default option. @@ -140,7 +143,8 @@ simple object to build the storage. It has 3 methods: storage or `undefined` if not found. - `clear() => MaybePromise`: - Clears all data from storage. + Clears all data from storage. **This method isn't used by the interceptor itself**, instead, its + here for you to use it programmatically. ## Third Party Storages @@ -240,7 +244,7 @@ const indexedDbStorage = buildStorage({ ### Node Cache -This example implementation uses [node-cache](https://github.com/node-cache/node-cache) as a storage method. Do note +This example implementation uses [node-cache](https://github.com/node-cache/node-cache) as a storage method. Do note that this library is somewhat old, however it appears to work at the time of writing. ```ts diff --git a/package.json b/package.json index 3b63ca27..ecbe38b9 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "axios-cache-interceptor", - "version": "1.6.0", + "version": "1.6.1", "description": "Cache interceptor for axios", "keywords": ["axios", "cache", "interceptor", "adapter", "http", "plugin", "wrapper"], "homepage": "https://axios-cache-interceptor.js.org", @@ -11,7 +11,6 @@ "author": "Arthur Fiorette ", "sideEffects": false, "type": "module", - "source": "./src/index.ts", "exports": { ".": { "require": "./dist/index.cjs", @@ -28,6 +27,7 @@ "jsdelivr": "./dist/index.bundle.js", "unpkg": "./dist/index.bundle.js", "module": "./dist/index.mjs", + "source": "./src/index.ts", "types": "./dist/index.d.ts", "scripts": { "benchmark": "cd benchmark && pnpm start", @@ -35,13 +35,13 @@ "docs:build": "vitepress build docs", "docs:dev": "vitepress dev docs --port 1227", "docs:serve": "vitepress serve docs", - "test": "c8 --reporter lcov --reporter text node --import ./test/setup.js --enable-source-maps --test test/**/*.test.ts", - "test:only": "c8 --reporter lcov --reporter text node --import ./test/setup.js --enable-source-maps --test-only", - "version": "auto-changelog -p && cp CHANGELOG.md docs/src/others/changelog.md && git add CHANGELOG.md docs/src/others/changelog.md", "format": "biome format --write .", "lint": "biome check .", + "lint:ci": "biome ci .", "lint:fix": "biome check --write --unsafe .", - "lint:ci": "biome ci ." + "test": "c8 --reporter lcov --reporter text node --import ./test/setup.js --enable-source-maps --test test/**/*.test.ts", + "test:only": "c8 --reporter lcov --reporter text node --import ./test/setup.js --enable-source-maps --test-only", + "version": "auto-changelog -p && cp CHANGELOG.md docs/src/others/changelog.md && git add CHANGELOG.md docs/src/others/changelog.md" }, "resolutions": { "colors": "1.4.0" @@ -52,6 +52,7 @@ "object-code": "1.3.3" }, "devDependencies": { + "@arthurfiorette/biomejs-config": "1.0.5", "@biomejs/biome": "1.9.4", "@swc-node/register": "1.9.0", "@swc/helpers": "0.5.13", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 6c4f658e..e8b38905 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -21,6 +21,9 @@ importers: specifier: 1.3.3 version: 1.3.3 devDependencies: + '@arthurfiorette/biomejs-config': + specifier: 1.0.5 + version: 1.0.5 '@biomejs/biome': specifier: 1.9.4 version: 1.9.4 @@ -129,6 +132,9 @@ packages: resolution: {integrity: sha512-lFMjJTrFL3j7L9yBxwYfCq2k6qqwHyzuUl/XBnif78PWTJYyL/dfowQHWE3sp6U6ZzqWiiIZnpTMO96zhkjwtg==} engines: {node: '>=6.0.0'} + '@arthurfiorette/biomejs-config@1.0.5': + resolution: {integrity: sha512-2+r6+zsme3IuIr3Vuba7oZyif748EYnFOPGm8kahY4AM8q9EzJZRWNfaAveuV0+PTrLaIhZPhUKIkAhg5JLQ5A==} + '@babel/code-frame@7.23.5': resolution: {integrity: sha512-CgH3s1a96LipHCmSUmYFPwY7MNx8C3avkq7i4Wl3cfa662ldtUe4VM1TPXX70pfmrlWTb6jLqTYrZyT2ZTJBgA==} engines: {node: '>=6.9.0'} @@ -3441,6 +3447,8 @@ snapshots: '@jridgewell/gen-mapping': 0.3.3 '@jridgewell/trace-mapping': 0.3.18 + '@arthurfiorette/biomejs-config@1.0.5': {} + '@babel/code-frame@7.23.5': dependencies: '@babel/highlight': 7.23.4 diff --git a/src/cache/cache.ts b/src/cache/cache.ts index bd0d3121..abf55bc1 100644 --- a/src/cache/cache.ts +++ b/src/cache/cache.ts @@ -4,7 +4,6 @@ import type { HeaderInterpreter } from '../header/types.js'; import type { AxiosInterceptor } from '../interceptors/build.js'; import type { AxiosStorage, - CachedResponse, CachedStorageValue, LoadingStorageValue, StaleStorageValue @@ -86,7 +85,7 @@ export interface CacheProperties { * We use `methods` in a per-request configuration setup because sometimes you have * exceptions to the method rule. * - * @default ['get'] + * @default ['get', 'head'] * @see https://axios-cache-interceptor.js.org/config/request-specifics#cache-methods */ methods: Lowercase[]; @@ -261,10 +260,10 @@ export interface CacheInstance { * You'd normally not need to change this, but it is exposed in case you need to use it * as some sort of listener of know when a request is waiting for other to finish. * - * @default { } + * @default new Map() * @see https://axios-cache-interceptor.js.org/config#waiting */ - waiting: Record>; + waiting: Map>; /** * The function used to interpret all headers from a request and determine a time to diff --git a/src/cache/create.ts b/src/cache/create.ts index 1a76d0a5..66fc3aa6 100644 --- a/src/cache/create.ts +++ b/src/cache/create.ts @@ -39,7 +39,7 @@ export function setupCache(axios: AxiosInstance, options: CacheOptions = {}): Ax throw new Error('Use buildStorage() function'); } - axiosCache.waiting = options.waiting || {}; + axiosCache.waiting = options.waiting || new Map(); axiosCache.generateKey = options.generateKey || defaultKeyGenerator; diff --git a/src/interceptors/request.ts b/src/interceptors/request.ts index a7c54086..388e995d 100644 --- a/src/interceptors/request.ts +++ b/src/interceptors/request.ts @@ -94,7 +94,7 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) { // This checks for simultaneous access to a new key. The js event loop jumps on the // first await statement, so the second (asynchronous call) request may have already // started executing. - if (axios.waiting[config.id] && !overrideCache) { + if (axios.waiting.has(config.id) && !overrideCache) { cache = (await axios.storage.get(config.id, config)) as | CachedStorageValue | LoadingStorageValue; @@ -116,11 +116,12 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) { } // Create a deferred to resolve other requests for the same key when it's completed - axios.waiting[config.id] = deferred(); + const def = deferred(); + axios.waiting.set(config.id, def); // Adds a default reject handler to catch when the request gets aborted without // others waiting for it. - axios.waiting[config.id]!.catch(() => undefined); + def.catch(() => undefined); await axios.storage.set( config.id, @@ -178,7 +179,7 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) { let cachedResponse: CachedResponse; if (cache.state === 'loading') { - const deferred = axios.waiting[config.id]; + const deferred = axios.waiting.get(config.id); // The deferred may not exists when the process is using a persistent // storage and cancelled in the middle of a request, this would result in @@ -200,7 +201,28 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) { } try { - cachedResponse = await deferred; + // Deferred can't reuse the value because the user's storage might clone + // or mutate the value, so we need to ask it again. + // For example with memoryStorage + cloneData + await deferred; + const state = await axios.storage.get(config.id, config); + + // This is a cache mismatch and should never happen, but in case it does, + // we need to redo the request all over again. + /* c8 ignore start */ + if (!state.data) { + if (__ACI_DEV__) { + axios.debug({ + id: config.id, + msg: 'Deferred resolved, but no data was found, requesting again' + }); + } + + return onFulfilled(config); + } + /* c8 ignore end */ + + cachedResponse = state.data; } catch (err) { if (__ACI_DEV__) { axios.debug({ @@ -211,10 +233,11 @@ export function defaultRequestInterceptor(axios: AxiosCacheInstance) { } // Hydrates any UI temporarily, if cache is available - /* c8 ignore next 3 */ + /* c8 ignore start */ if (cache.data) { await config.cache.hydrate?.(cache); } + /* c8 ignore end */ // The deferred is rejected when the request that we are waiting rejects its cache. // In this case, we need to redo the request all over again. diff --git a/src/interceptors/response.ts b/src/interceptors/response.ts index 0f6d291e..8361f84b 100644 --- a/src/interceptors/response.ts +++ b/src/interceptors/response.ts @@ -20,9 +20,12 @@ export function defaultResponseInterceptor(axios: AxiosCacheInstance): ResponseI await axios.storage.remove(responseId, config); // Rejects the deferred, if present - axios.waiting[responseId]?.reject(); + const deferred = axios.waiting.get(responseId); - delete axios.waiting[responseId]; + if (deferred) { + deferred.reject(); + axios.waiting.delete(responseId); + } }; const onFulfilled: ResponseInterceptor['onFulfilled'] = async (response) => { @@ -200,12 +203,15 @@ export function defaultResponseInterceptor(axios: AxiosCacheInstance): ResponseI data }; + // Define this key as cache on the storage + await axios.storage.set(response.id, newCache, config); + // Resolve all other requests waiting for this response - const waiting = axios.waiting[response.id]; + const waiting = axios.waiting.get(response.id); if (waiting) { - waiting.resolve(newCache.data); - delete axios.waiting[response.id]; + waiting.resolve(); + axios.waiting.delete(response.id); if (__ACI_DEV__) { axios.debug({ @@ -215,9 +221,6 @@ export function defaultResponseInterceptor(axios: AxiosCacheInstance): ResponseI } } - // Define this key as cache on the storage - await axios.storage.set(response.id, newCache, config); - if (__ACI_DEV__) { axios.debug({ id: response.id, @@ -323,10 +326,6 @@ export function defaultResponseInterceptor(axios: AxiosCacheInstance): ResponseI // staleIfError is the number of seconds that stale is allowed to be used (typeof staleIfError === 'number' && cache.createdAt + staleIfError > Date.now()) ) { - // Resolve all other requests waiting for this response - axios.waiting[id]?.resolve(cache.data); - delete axios.waiting[id]; - // re-mark the cache as stale await axios.storage.set( id, @@ -337,6 +336,20 @@ export function defaultResponseInterceptor(axios: AxiosCacheInstance): ResponseI }, config ); + // Resolve all other requests waiting for this response + const waiting = axios.waiting.get(id); + + if (waiting) { + waiting.resolve(); + axios.waiting.delete(id); + + if (__ACI_DEV__) { + axios.debug({ + id, + msg: 'Found waiting deferred(s) and resolved them' + }); + } + } if (__ACI_DEV__) { axios.debug({ diff --git a/src/storage/build.ts b/src/storage/build.ts index b599ee2b..a8eda818 100644 --- a/src/storage/build.ts +++ b/src/storage/build.ts @@ -65,13 +65,6 @@ export interface BuildStorage extends Omit { key: string, currentRequest?: CacheRequestConfig ) => MaybePromise; - - /** - * Deletes all values from the storage. - * - * @see https://axios-cache-interceptor.js.org/guide/storages#buildstorage - */ - clear: () => MaybePromise; } /** diff --git a/src/storage/memory.ts b/src/storage/memory.ts index 83b4aed8..131f6260 100644 --- a/src/storage/memory.ts +++ b/src/storage/memory.ts @@ -1,12 +1,17 @@ import { buildStorage, canStale, isExpired } from './build.js'; -import type { AxiosStorage, NotEmptyStorageValue, StorageValue } from './types.js'; +import type { AxiosStorage, StorageValue } from './types.js'; +/* c8 ignore start */ /** - * Modern function to natively deep clone. - * - * @link https://caniuse.com/mdn-api_structuredclone (07/03/2022 -> 59.4%) + * Clones an object using the structured clone algorithm if available, otherwise + * it uses JSON.parse(JSON.stringify(value)). */ -declare const structuredClone: ((value: T) => T) | undefined; +const clone: (value: T) => T = + // https://caniuse.com/mdn-api_structuredclone (10/18/2023 92.51%) + typeof structuredClone === 'function' + ? structuredClone + : (value) => JSON.parse(JSON.stringify(value)); +/* c8 ignore stop */ /** * Creates a simple in-memory storage. This means that if you need to persist data between @@ -69,15 +74,9 @@ export function buildMemoryStorage( } } - storage.data[key] = - // Clone the value before storing to prevent future mutations - // from affecting cached data. - cloneData === 'double' - ? /* c8 ignore next 3 */ - typeof structuredClone === 'function' - ? structuredClone(value) - : (JSON.parse(JSON.stringify(value)) as NotEmptyStorageValue) - : value; + // Clone the value before storing to prevent future mutations + // from affecting cached data. + storage.data[key] = cloneData === 'double' ? clone(value) : value; }, remove: (key) => { @@ -87,16 +86,7 @@ export function buildMemoryStorage( find: (key) => { const value = storage.data[key]; - /* c8 ignore next 7 */ - if (cloneData && value !== undefined) { - if (typeof structuredClone === 'function') { - return structuredClone(value); - } - - return JSON.parse(JSON.stringify(value)) as StorageValue; - } - - return value; + return cloneData && value !== undefined ? clone(value) : value; }, clear: () => { @@ -123,8 +113,6 @@ export function buildMemoryStorage( value = storage.data[key]!; if (value.state === 'empty') { - // this storage returns void. - storage.remove(key); continue; } diff --git a/src/storage/types.ts b/src/storage/types.ts index e478336b..1f3c3ce5 100644 --- a/src/storage/types.ts +++ b/src/storage/types.ts @@ -136,9 +136,12 @@ export interface AxiosStorage { get: (key: string, currentRequest?: CacheRequestConfig) => MaybePromise; /** - * Deletes all values from the storage. + * Deletes all values from the storage, this method isn't used by the interceptor + * and is here just for convenience. + * + * **All native storages implement them, but it's not required.** * * @see https://axios-cache-interceptor.js.org/guide/storages#buildstorage */ - clear: () => MaybePromise; + clear?: () => MaybePromise; } diff --git a/test/interceptors/request.test.ts b/test/interceptors/request.test.ts index d153d5c5..f456e4f8 100644 --- a/test/interceptors/request.test.ts +++ b/test/interceptors/request.test.ts @@ -4,6 +4,7 @@ import { setTimeout } from 'node:timers/promises'; import type { AxiosAdapter, AxiosResponse } from 'axios'; import type { CacheRequestConfig, InternalCacheRequestConfig } from '../../src/cache/axios.js'; import { Header } from '../../src/header/headers.js'; +import { buildMemoryStorage } from '../../src/index.js'; import type { LoadingStorageValue } from '../../src/storage/types.js'; import { mockAxios } from '../mocks/axios.js'; import { mockDateNow } from '../utils.js'; @@ -227,7 +228,7 @@ describe('Request Interceptor', () => { // it still has a waiting entry. const { state } = await axios.storage.get(ID); assert.equal(state, 'empty'); - assert.ok(axios.waiting[ID]); + assert.ok(axios.waiting.get(ID)); // This line should throw an error if this bug isn't fixed. await axios.get('url', { id: ID }); @@ -235,7 +236,7 @@ describe('Request Interceptor', () => { const { state: newState } = await axios.storage.get(ID); assert.notEqual(newState, 'empty'); - assert.equal(axios.waiting[ID], undefined); + assert.equal(axios.waiting.get(ID), undefined); }); it('`cache.override = true` with previous cache', async () => { @@ -451,4 +452,40 @@ describe('Request Interceptor', () => { assert.equal(req5.cached, false); assert.equal(req5.stale, undefined); }); + + it('clone works with concurrent requests', async () => { + const axios = mockAxios( + { + storage: buildMemoryStorage('double') + }, + undefined, + undefined, + () => ({ a: 1 }) + ); + + await Promise.all( + Array.from({ length: 10 }, async () => { + const result = await axios.get<{ a: 1 }>('/url'); + result.data.a++; + assert.equal(result.data.a, 2); + }) + ); + }); + + it('clone works with sequential requests', async () => { + const axios = mockAxios( + { + storage: buildMemoryStorage('double') + }, + undefined, + undefined, + () => ({ a: 1 }) + ); + + for (let i = 0; i < 10; i++) { + const result = await axios.get<{ a: 1 }>('/url'); + result.data.a++; + assert.equal(result.data.a, 2); + } + }); }); diff --git a/test/mocks/axios.ts b/test/mocks/axios.ts index acd0fe5e..bc5bb809 100644 --- a/test/mocks/axios.ts +++ b/test/mocks/axios.ts @@ -9,7 +9,8 @@ export const XMockRandom = 'x-mock-random'; export function mockAxios( options: CacheOptions = {}, responseHeaders: Record = {}, - instance = Axios.create() + instance = Axios.create(), + data: () => any = () => true ): AxiosCacheInstance { const axios = setupCache(instance, options); @@ -30,7 +31,7 @@ export function mockAxios( config, { config }, { - data: true, + data: data(), status, statusText, headers: { @@ -45,7 +46,7 @@ export function mockAxios( } return { - data: true, + data: data(), status, statusText, headers: {