Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fixes cases re throttledPromised: abort misbehaviour and not awaited queue #865

Closed
wants to merge 8 commits into from
48 changes: 48 additions & 0 deletions src/throttlePromise.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
import { describe, it, expect, 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<any>[] = []

// 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)
})
})
27 changes: 17 additions & 10 deletions src/throttlePromise.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ type Queue = {

interface ISbThrottle {
abort: () => any
(args: []): Promise<Queue>
(...args: any): Promise<Queue>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking: @alexjoverm Can we do anything to avoid using any here? I will leave it to your personal criteria if it's something we should do on the scope of this PR or in the v7 refactor. In normal cases I would block a PR from merging if there are any on the types

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do, will need some extra help as I think we can do this type a bit more precise

name: string
AbortError?: () => void
}
Expand All @@ -40,28 +40,27 @@ function throttledQueue(fn: ThrottleFn, limit: number, interval: number) {
const queue: Queue[] = []
let timeouts: ReturnType<typeof setTimeout>[] = []
let activeCount = 0
let isAborted = false

const next = function () {
const next = async function () {
activeCount++

const x = queue.shift() as unknown as Shifted
x.resolve(await fn.apply(x.self, x.args))

const id = setTimeout(function () {
activeCount--

if (queue.length > 0) {
next()
}

timeouts = timeouts.filter(function (currentId) {
return currentId !== id
})
timeouts = timeouts.filter((currentId) => currentId !== id)
}, interval)

if (timeouts.indexOf(id) < 0) {
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 (
Expand All @@ -70,8 +69,15 @@ function throttledQueue(fn: ThrottleFn, limit: number, interval: number) {
): Promise<Queue> {
// eslint-disable-next-line @typescript-eslint/no-this-alias
const self = this
if (isAborted) {
return Promise.reject(
new Error(
'Throttled function is aborted and not accepting new promises'
)
)
}

return new Promise(function (resolve, reject) {
return new Promise((resolve, reject) => {
queue.push({
resolve: resolve,
reject: reject,
Expand All @@ -86,6 +92,7 @@ function throttledQueue(fn: ThrottleFn, limit: number, interval: number) {
}

throttled.abort = function () {
isAborted = true
timeouts.forEach(clearTimeout)
timeouts = []

Expand Down
4 changes: 2 additions & 2 deletions tests/api/index.e2e.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import StoryblokClient from 'storyblok-js-client'
import { describe, it, expect, beforeEach } from 'vitest'

describe('StoryblokClient', () => {
let client
let client: StoryblokClient

beforeEach(() => {
// Setup default mocks
Expand Down Expand Up @@ -52,7 +52,7 @@ describe('StoryblokClient', () => {

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'
version: 'draft',
})
expect(data.story.slug).toBe('testcontent-draft')
})
Expand Down
4 changes: 2 additions & 2 deletions tests/utils.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
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
}
Expand Down
7 changes: 5 additions & 2 deletions tsconfig.json
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,14 @@
"declaration": true,
"declarationDir": "dist/types",
"emitDeclarationOnly": true,
"lib": ["ESNext", "DOM", "DOM.Iterable"]
"lib": ["ESNext", "DOM", "DOM.Iterable"],
// "paths": {
// "storyblok-js-client": ["./dist/types/index.d.ts"],
// },
},
"extends": "@tsconfig/recommended/tsconfig.json",
"$schema": "https://json.schemastore.org/tsconfig",
"display": "Recommended",
"include": ["./src"],
"exclude": ["node_modules", "./src/**/*.test.ts", "./src/**/*.spec.ts"]
"exclude": ["node_modules", "./src/**/*.test.ts", "./src/**/*.spec.ts"],
}
6 changes: 6 additions & 0 deletions vitest.config.e2e.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,14 @@
import { defineConfig } from 'vite'
import path from 'path'

export default defineConfig({
test: {
include: ['./tests/**/*.e2e.ts'],
setupFiles: ['./tests/setup.js'],
},
resolve: {
alias: {
'storyblok-js-client': path.resolve(__dirname, 'dist'),
},
},
})