From 5698b6e29ffd1a037d625ca10295c0bdc0be8a55 Mon Sep 17 00:00:00 2001 From: Alex Jover Date: Fri, 15 Nov 2024 18:23:30 +0100 Subject: [PATCH 1/2] fix: update throttle --- .github/workflows/lint.yml | 2 +- src/index.ts | 42 ++++++---- src/interfaces.ts | 31 ++++++- src/throttlePromise.test.ts | 48 +++++++++++ src/throttlePromise.ts | 81 +++++++++--------- tests/api/index.e2e.ts | 160 ++++++++++++++++++------------------ tests/setup.js | 2 +- tests/utils.ts | 8 +- tsconfig.json | 14 ++-- vite.build.mjs | 14 ++-- vite.config.ts | 14 ++-- vitest.config.e2e.ts | 10 ++- vitest.config.ts | 4 +- 13 files changed, 260 insertions(+), 170 deletions(-) create mode 100644 src/throttlePromise.test.ts diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 4588f051..02483cf6 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -23,4 +23,4 @@ jobs: - name: Install dependencies run: pnpm install - name: Run Lint - run: pnpm run lint \ No newline at end of file + run: pnpm run lint diff --git a/src/index.ts b/src/index.ts index 47d92f78..d9183210 100755 --- a/src/index.ts +++ b/src/index.ts @@ -14,13 +14,14 @@ import type { ISbCustomFetch, ISbLinkURLObject, ISbNode, + ISbResponse, + ISbResponseData, ISbResult, ISbStories, ISbStoriesParams, ISbStory, ISbStoryData, ISbStoryParams, - ThrottleFn, } from './interfaces'; let memory: Partial = {}; @@ -47,15 +48,6 @@ interface ISbFlatMapped { data: any; } -export interface ISbResponseData { - link_uuids: string[]; - links: string[]; - rel_uuids: string[]; - rels: any; - story: ISbStoryData; - stories: Array; -} - const _VERSION = { V1: 'v1', V2: 'v2', @@ -68,7 +60,7 @@ class Storyblok { private client: SbFetch; private maxRetries: number; private retriesDelay: number; - private throttle: ThrottleFn; + private throttle; private accessToken: string; private cache: ISbCache; private helpers: SbHelpers; @@ -148,7 +140,11 @@ class Storyblok { this.maxRetries = config.maxRetries || 10; this.retriesDelay = 300; - this.throttle = throttledQueue(this.throttledRequest, rateLimit, 1000); + this.throttle = throttledQueue( + this.throttledRequest.bind(this), + rateLimit, + 1000, + ); this.accessToken = config.accessToken || ''; this.relations = {} as RelationsType; this.links = {} as LinksType; @@ -281,7 +277,9 @@ class Storyblok { ): Promise { const url = `/${slug}`; - return Promise.resolve(this.throttle('post', url, params, fetchOptions)); + return Promise.resolve( + this.throttle('post', url, params, fetchOptions), + ) as Promise; } public put( @@ -291,7 +289,9 @@ class Storyblok { ): Promise { const url = `/${slug}`; - return Promise.resolve(this.throttle('put', url, params, fetchOptions)); + return Promise.resolve( + this.throttle('put', url, params, fetchOptions), + ) as Promise; } public delete( @@ -301,7 +301,9 @@ class Storyblok { ): Promise { const url = `/${slug}`; - return Promise.resolve(this.throttle('delete', url, params, fetchOptions)); + return Promise.resolve( + this.throttle('delete', url, params, fetchOptions), + ) as Promise; } public getStories( @@ -616,7 +618,12 @@ class Storyblok { return new Promise(async (resolve, reject) => { try { - const res = await this.throttle('get', url, params, fetchOptions); + const res = (await this.throttle( + 'get', + url, + params, + fetchOptions, + )) as ISbResponse; if (res.status !== 200) { return reject(res); } @@ -635,7 +642,8 @@ class Storyblok { } if (response.data.story || response.data.stories) { - const resolveId = (this.resolveCounter = ++this.resolveCounter % 1000); + const resolveId = (this.resolveCounter + = ++this.resolveCounter % 1000); await this.resolveStories(response.data, params, `${resolveId}`); } diff --git a/src/interfaces.ts b/src/interfaces.ts index f3f5704f..a0e291c8 100644 --- a/src/interfaces.ts +++ b/src/interfaces.ts @@ -1,4 +1,5 @@ import type { ResponseFn } from './sbFetch'; +import type Method from './constants'; export interface ISbStoriesParams extends Partial, @@ -241,6 +242,7 @@ export interface ISbResponse { data: any; status: number; statusText: string; + headers: any; } export interface ISbError { @@ -339,10 +341,35 @@ export interface ISbLinks { }; } -export interface ThrottleFn { - (...args: any): any; +export interface Queue { + resolve: (value: unknown) => void; + reject: (reason?: unknown) => void; + args: T; +} + +export interface ISbResponseData { + link_uuids: string[]; + links: string[]; + rel_uuids: string[]; + rels: any; + story: ISbStoryData; + stories: Array; } +export interface ISbThrottle< + T extends (...args: Parameters) => ReturnType, +> { + abort?: () => void; + (...args: Parameters): Promise; +} + +export type ISbThrottledRequest = ( + type: Method, + url: string, + params: ISbStoriesParams, + fetchOptions?: ISbCustomFetch +) => Promise; + export type AsyncFn = (...args: any) => [] | Promise; export type ArrayFn = (...args: any) => void; diff --git a/src/throttlePromise.test.ts b/src/throttlePromise.test.ts new file mode 100644 index 00000000..325d275a --- /dev/null +++ b/src/throttlePromise.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it, vi } from 'vitest'; +import throttledQueue from './throttlePromise'; + +// Mock function to simulate async work with a delay +const mockFn = vi.fn(async (input) => { + await new Promise(resolve => setTimeout(resolve, 200)); // Simulate async delay + return input; +}); + +describe('throttledQueue', () => { + it('should resolve or reject all promises after the queue finishes, even when aborting', async () => { + const throttled = throttledQueue(mockFn, 3, 10); // Throttle with 3 concurrent tasks + const promises: Promise[] = []; + + // Generate 10 tasks and push them to the promises array + for (let i = 0; i < 10; i++) { + promises.push(throttled(i)); + if (i === 5) { + throttled.abort(); // but abort at call #6 + } + } + + const results = await Promise.allSettled(promises); + results.forEach((result) => { + expect(['fulfilled', 'rejected']).toContain(result.status); + }); + }); + it('should enforce sequential resolution when throttle limit is exceeded', async () => { + const throttled = throttledQueue(mockFn, 1, 100); // Limit of 1, 100ms interval + + const start = Date.now(); + const promises = [ + throttled('test1'), + throttled('test2'), + throttled('test3'), + ]; + + const results = await Promise.all(promises); + const duration = Date.now() - start; + + // Expected behavior: + // Since each call has a 200ms delay, and there's a 100ms throttle interval and limit is 1, + // and each successive call should only start after the previous one completes, + // then the total duration should be around 800ms (200*3 + 100*2). + expect(results).toEqual(['test1', 'test2', 'test3']); + expect(duration).toBeGreaterThanOrEqual(800); + }); +}); diff --git a/src/throttlePromise.ts b/src/throttlePromise.ts index e24da82c..dda74d98 100644 --- a/src/throttlePromise.ts +++ b/src/throttlePromise.ts @@ -1,41 +1,47 @@ -import type { ThrottleFn } from './interfaces'; +import type { ISbThrottle, Queue } from './interfaces'; -interface Shifted { - args: any; - self: any; - resolve: (args: any) => any; +class AbortError extends Error { + constructor(msg: string) { + super(msg); + this.name = 'AbortError'; + } } -interface Queue { - resolve: (args: any) => any; - reject: (args: any) => any; - args: any[]; - self: any; -} +function isFinite(value: number) { + if (Number.isNaN(value) || value === Infinity || value === -Infinity) { + return false; + } -interface ISbThrottle { - abort: () => any; - (args: []): Promise; - name: string; - AbortError?: () => void; + return true; } -function throttledQueue(fn: ThrottleFn, limit: number, interval: number) { - if (!Number.isFinite(limit)) { +function throttledQueue) => ReturnType>( + fn: T, + limit: number, + interval: number, +): ISbThrottle { + if (!isFinite(limit)) { throw new TypeError('Expected `limit` to be a finite number'); } - if (!Number.isFinite(interval)) { + if (!isFinite(interval)) { throw new TypeError('Expected `interval` to be a finite number'); } - const queue: Queue[] = []; + const queue: Queue>[] = []; let timeouts: ReturnType[] = []; let activeCount = 0; + let isAborted = false; - const next = function () { + const next = async () => { activeCount++; + const x = queue.shift(); + if (x) { + const res = await fn(...x.args); + x.resolve(res); + } + const id = setTimeout(() => { activeCount--; @@ -43,31 +49,28 @@ function throttledQueue(fn: ThrottleFn, limit: number, interval: number) { next(); } - timeouts = timeouts.filter((currentId) => { - return currentId !== id; - }); + timeouts = timeouts.filter(currentId => currentId !== id); }, interval); if (!timeouts.includes(id)) { timeouts.push(id); } - - const x = queue.shift() as unknown as Shifted; - x.resolve(fn.apply(x.self, x.args)); }; - const throttled: ISbThrottle = function ( - this: ISbThrottle, - ...args: [] - ): Promise { - const self = this; + const throttled: ISbThrottle = (...args) => { + if (isAborted) { + return Promise.reject( + new Error( + 'Throttled function is already aborted and not accepting new promises', + ), + ); + } return new Promise((resolve, reject) => { queue.push({ resolve, reject, args, - self, }); if (activeCount < limit) { @@ -76,16 +79,14 @@ function throttledQueue(fn: ThrottleFn, limit: number, interval: number) { }); }; - throttled.abort = function () { + throttled.abort = () => { + isAborted = true; timeouts.forEach(clearTimeout); timeouts = []; - queue.forEach((x) => { - x.reject(function (this: ISbThrottle) { - Error.call(this, 'Throttled function aborted'); - this.name = 'AbortError'; - }); - }); + queue.forEach(x => + x.reject(() => new AbortError('Throttle function aborted')), + ); queue.length = 0; }; diff --git a/tests/api/index.e2e.ts b/tests/api/index.e2e.ts index 5c02ec6a..8afd3809 100644 --- a/tests/api/index.e2e.ts +++ b/tests/api/index.e2e.ts @@ -1,16 +1,16 @@ -import StoryblokClient from 'storyblok-js-client' -import { describe, it, expect, beforeEach } from 'vitest' +import StoryblokClient from 'storyblok-js-client'; +import { beforeEach, describe, expect, it } from 'vitest'; describe('StoryblokClient', () => { - let client + let client: StoryblokClient; beforeEach(() => { // Setup default mocks client = new StoryblokClient({ accessToken: process.env.VITE_ACCESS_TOKEN, cache: { type: 'memory', clear: 'auto' }, - }) - }) + }); + }); // TODO: Uncomment when we have a valid token /* if (process.env.VITE_OAUTH_TOKEN) { describe('management API', () => { @@ -28,72 +28,72 @@ describe('StoryblokClient', () => { } */ describe('get function', () => { - it("get('cdn/spaces/me') should return the space information", async () => { - const { data } = await client.get('cdn/spaces/me') - expect(data.space.id).toBe(Number(process.env.VITE_SPACE_ID)) - }) - - it("get('cdn/stories') should return all stories", async () => { - const { data } = await client.get('cdn/stories') - expect(data.stories.length).toBeGreaterThan(0) - }) - - it("get('cdn/stories/testcontent-0' should return the specific story", async () => { - const { data } = await client.get('cdn/stories/testcontent-0') - expect(data.story.slug).toBe('testcontent-0') - }) - - it("get('cdn/stories' { starts_with: testcontent-0 } should return the specific story", async () => { + it('get(\'cdn/spaces/me\') should return the space information', async () => { + const { data } = await client.get('cdn/spaces/me'); + expect(data.space.id).toBe(Number(process.env.VITE_SPACE_ID)); + }); + + it('get(\'cdn/stories\') should return all stories', async () => { + const { data } = await client.get('cdn/stories'); + expect(data.stories.length).toBeGreaterThan(0); + }); + + it('get(\'cdn/stories/testcontent-0\' should return the specific story', async () => { + const { data } = await client.get('cdn/stories/testcontent-0'); + expect(data.story.slug).toBe('testcontent-0'); + }); + + it('get(\'cdn/stories\' { starts_with: testcontent-0 } should return the specific story', async () => { const { data } = await client.get('cdn/stories', { starts_with: 'testcontent-0', - }) - expect(data.stories.length).toBe(1) - }) + }); + expect(data.stories.length).toBe(1); + }); - it("get('cdn/stories/testcontent-draft', { version: 'draft' }) should return the specific story draft", async () => { + it('get(\'cdn/stories/testcontent-draft\', { version: \'draft\' }) should return the specific story draft', async () => { const { data } = await client.get('cdn/stories/testcontent-draft', { version: 'draft', - }) - expect(data.story.slug).toBe('testcontent-draft') - }) + }); + expect(data.story.slug).toBe('testcontent-draft'); + }); - it("get('cdn/stories/testcontent-0', { version: 'published' }) should return the specific story published", async () => { + it('get(\'cdn/stories/testcontent-0\', { version: \'published\' }) should return the specific story published', async () => { const { data } = await client.get('cdn/stories/testcontent-0', { version: 'published', - }) - expect(data.story.slug).toBe('testcontent-0') - }) + }); + expect(data.story.slug).toBe('testcontent-0'); + }); it('cdn/stories/testcontent-0 should resolve author relations', async () => { const { data } = await client.get('cdn/stories/testcontent-0', { resolve_relations: 'root.author', - }) - console.log(data) - expect(data.story.content.author[0].slug).toBe('edgar-allan-poe') - }) + }); + + expect(data.story.content.author[0].slug).toBe('edgar-allan-poe'); + }); - it("get('cdn/stories', { by_slugs: 'folder/*' }) should return the specific story", async () => { + it('get(\'cdn/stories\', { by_slugs: \'folder/*\' }) should return the specific story', async () => { const { data } = await client.get('cdn/stories', { by_slugs: 'folder/*', - }) - expect(data.stories.length).toBeGreaterThan(0) - }) - }) + }); + expect(data.stories.length).toBeGreaterThan(0); + }); + }); describe('getAll function', () => { - it("getAll('cdn/stories') should return all stories", async () => { - const result = await client.getAll('cdn/stories') - expect(result.length).toBeGreaterThan(0) - }) + it('getAll(\'cdn/stories\') should return all stories', async () => { + const result = await client.getAll('cdn/stories'); + expect(result.length).toBeGreaterThan(0); + }); - it("getAll('cdn/stories') should return all stories with filtered results", async () => { + it('getAll(\'cdn/stories\') should return all stories with filtered results', async () => { const result = await client.getAll('cdn/stories', { starts_with: 'testcontent-0', - }) - expect(result.length).toBe(1) - }) + }); + expect(result.length).toBe(1); + }); - it("getAll('cdn/stories', filter_query: { __or: [{ category: { any_in_array: 'Category 1' } }, { category: { any_in_array: 'Category 2' } }]}) should return all stories with the specific filter applied", async () => { + it('getAll(\'cdn/stories\', filter_query: { __or: [{ category: { any_in_array: \'Category 1\' } }, { category: { any_in_array: \'Category 2\' } }]}) should return all stories with the specific filter applied', async () => { const result = await client.getAll('cdn/stories', { filter_query: { __or: [ @@ -101,47 +101,47 @@ describe('StoryblokClient', () => { { category: { any_in_array: 'Category 2' } }, ], }, - }) - expect(result.length).toBeGreaterThan(0) - }) + }); + expect(result.length).toBeGreaterThan(0); + }); - it("getAll('cdn/stories', {by_slugs: 'folder/*'}) should return all stories with the specific filter applied", async () => { + it('getAll(\'cdn/stories\', {by_slugs: \'folder/*\'}) should return all stories with the specific filter applied', async () => { const result = await client.getAll('cdn/stories', { by_slugs: 'folder/*', - }) - expect(result.length).toBeGreaterThan(0) - }) + }); + expect(result.length).toBeGreaterThan(0); + }); - it("getAll('cdn/links') should return all links", async () => { - const result = await client.getAll('cdn/links') - expect(result.length).toBeGreaterThan(0) - }) - }) + it('getAll(\'cdn/links\') should return all links', async () => { + const result = await client.getAll('cdn/links'); + expect(result.length).toBeGreaterThan(0); + }); + }); describe('caching', () => { - it("get('cdn/spaces/me') should not be cached", async () => { - const provider = client.cacheProvider() - await provider.flush() - await client.get('cdn/spaces/me') - expect(Object.values(provider.getAll()).length).toBe(0) - }) + it('get(\'cdn/spaces/me\') should not be cached', async () => { + const provider = client.cacheProvider(); + await provider.flush(); + await client.get('cdn/spaces/me'); + expect(Object.values(provider.getAll()).length).toBe(0); + }); - it("get('cdn/stories') should be cached when is a published version", async () => { - const cacheVersion = client.cacheVersion() + it('get(\'cdn/stories\') should be cached when is a published version', async () => { + const cacheVersion = client.cacheVersion(); - await client.get('cdn/stories') + await client.get('cdn/stories'); - expect(cacheVersion).not.toBe(undefined) + expect(cacheVersion).not.toBe(undefined); - const newCacheVersion = client.cacheVersion() + const newCacheVersion = client.cacheVersion(); - await client.get('cdn/stories') + await client.get('cdn/stories'); - expect(newCacheVersion).toBe(client.cacheVersion()) + expect(newCacheVersion).toBe(client.cacheVersion()); - await client.get('cdn/stories') + await client.get('cdn/stories'); - expect(newCacheVersion).toBe(client.cacheVersion()) - }) - }) -}) + expect(newCacheVersion).toBe(client.cacheVersion()); + }); + }); +}); diff --git a/tests/setup.js b/tests/setup.js index 855c2532..9f4b4e80 100644 --- a/tests/setup.js +++ b/tests/setup.js @@ -1 +1 @@ -import 'isomorphic-fetch' +import 'isomorphic-fetch'; diff --git a/tests/utils.ts b/tests/utils.ts index 2afa9023..caf165b9 100644 --- a/tests/utils.ts +++ b/tests/utils.ts @@ -1,7 +1,7 @@ -export function headersToObject(headers) { - const obj = {} +export function headersToObject(headers: Headers) { + const obj: { [key: string]: string } = {}; for (const [key, value] of headers.entries()) { - obj[key] = value + obj[key] = value; } - return obj + return obj; } diff --git a/tsconfig.json b/tsconfig.json index e1e45077..e1f60cc1 100644 --- a/tsconfig.json +++ b/tsconfig.json @@ -1,18 +1,18 @@ { + "extends": "@tsconfig/recommended/tsconfig.json", "compilerOptions": { - "module": "commonjs", - "isolatedModules": true, "target": "ESNext", + "lib": ["ESNext", "DOM", "DOM.Iterable"], + "module": "commonjs", "strict": true, - "esModuleInterop": true, - "skipLibCheck": true, - "forceConsistentCasingInFileNames": true, "declaration": true, "declarationDir": "dist/types", "emitDeclarationOnly": true, - "lib": ["ESNext", "DOM", "DOM.Iterable"] + "esModuleInterop": true, + "forceConsistentCasingInFileNames": true, + "isolatedModules": true, + "skipLibCheck": true }, - "extends": "@tsconfig/recommended/tsconfig.json", "$schema": "https://json.schemastore.org/tsconfig", "display": "Recommended", "include": ["./src"], diff --git a/vite.build.mjs b/vite.build.mjs index 38555a87..409b3938 100644 --- a/vite.build.mjs +++ b/vite.build.mjs @@ -1,10 +1,10 @@ -import { fileURLToPath } from 'node:url' -import { resolve } from 'node:path' -import { build } from 'vite' +import { fileURLToPath } from 'node:url'; +import { resolve } from 'node:path'; +import { build } from 'vite'; -const __dirname = fileURLToPath(new URL('.', import.meta.url)) +const __dirname = fileURLToPath(new URL('.', import.meta.url)); -let firstRunCounter = 0 +let firstRunCounter = 0; const bundles = [ { entry: 'entry.esm.ts', @@ -49,6 +49,6 @@ const bundles = [ npm_package_version: process.env.npm_package_version, }, }, - }) + }); } -})() +})(); diff --git a/vite.config.ts b/vite.config.ts index e2470a2d..8879c725 100644 --- a/vite.config.ts +++ b/vite.config.ts @@ -1,13 +1,13 @@ /// -import { defineConfig } from 'vite' -import { lightGreen } from 'kolorist' +import { defineConfig } from 'vite'; +import { lightGreen } from 'kolorist'; -import pkg from './package.json' -import banner from 'vite-plugin-banner' -import dts from 'vite-plugin-dts' +import pkg from './package.json'; +import banner from 'vite-plugin-banner'; +import dts from 'vite-plugin-dts'; // eslint-disable-next-line no-console -console.log(`${lightGreen('Storyblok Richtext')} v${pkg.version}`) +console.log(`${lightGreen('Storyblok Richtext')} v${pkg.version}`); export default defineConfig(() => ({ plugins: [ @@ -18,4 +18,4 @@ export default defineConfig(() => ({ content: `/**\n * name: ${pkg.name}\n * (c) ${new Date().getFullYear()}\n * description: ${pkg.description}\n * author: ${pkg.author}\n */`, }), ], -})) +})); diff --git a/vitest.config.e2e.ts b/vitest.config.e2e.ts index dcdc8643..7a1f8a97 100644 --- a/vitest.config.e2e.ts +++ b/vitest.config.e2e.ts @@ -1,8 +1,14 @@ -import { defineConfig } from 'vite' +import { defineConfig } from 'vite'; +import path from 'node:path'; export default defineConfig({ test: { include: ['./tests/**/*.e2e.ts'], setupFiles: ['./tests/setup.js'], }, -}) + resolve: { + alias: { + 'storyblok-js-client': path.resolve(__dirname, 'dist'), + }, + }, +}); diff --git a/vitest.config.ts b/vitest.config.ts index c8d30041..5997512b 100644 --- a/vitest.config.ts +++ b/vitest.config.ts @@ -1,4 +1,4 @@ -import { defineConfig } from 'vite' +import { defineConfig } from 'vite'; export default defineConfig({ test: { @@ -10,4 +10,4 @@ export default defineConfig({ reportsDirectory: './tests/unit/coverage', }, }, -}) +}); From 1b83ebbab64e0d1d4b81cc90d04ee54e4f093922 Mon Sep 17 00:00:00 2001 From: Alex Jover Date: Tue, 19 Nov 2024 07:00:28 +0100 Subject: [PATCH 2/2] chore: iterate types and isFinite --- src/index.ts | 3 ++- src/throttlePromise.ts | 12 ++---------- 2 files changed, 4 insertions(+), 11 deletions(-) diff --git a/src/index.ts b/src/index.ts index d9183210..6e82a7c3 100755 --- a/src/index.ts +++ b/src/index.ts @@ -60,7 +60,7 @@ class Storyblok { private client: SbFetch; private maxRetries: number; private retriesDelay: number; - private throttle; + private throttle: ReturnType; private accessToken: string; private cache: ISbCache; private helpers: SbHelpers; @@ -145,6 +145,7 @@ class Storyblok { rateLimit, 1000, ); + this.accessToken = config.accessToken || ''; this.relations = {} as RelationsType; this.links = {} as LinksType; diff --git a/src/throttlePromise.ts b/src/throttlePromise.ts index dda74d98..45cbc583 100644 --- a/src/throttlePromise.ts +++ b/src/throttlePromise.ts @@ -7,24 +7,16 @@ class AbortError extends Error { } } -function isFinite(value: number) { - if (Number.isNaN(value) || value === Infinity || value === -Infinity) { - return false; - } - - return true; -} - function throttledQueue) => ReturnType>( fn: T, limit: number, interval: number, ): ISbThrottle { - if (!isFinite(limit)) { + if (!Number.isFinite(limit)) { throw new TypeError('Expected `limit` to be a finite number'); } - if (!isFinite(interval)) { + if (!Number.isFinite(interval)) { throw new TypeError('Expected `interval` to be a finite number'); }