Repository: tinylibs/tinybench
Pull request #259 (2025-03-17 17:24:57)

feat: add mode to fn hooks
Provide a way to check the mode for fn hooks that is consistent with setup and teardown.

crimx: > The idea and initial implementation are ok, but lack:
> 
> * existing JSDoc update
> * documentation update
> * dedicated unit tests testing that callbacks mode is respected
> 
> Since it's a breaking change in the current PR implementation, a backward compatible implementation should be investigated.

- I noticed the JSDoc artifacts are in the source code. Should I build the JSDoc manually? That would be hard to review.
- There is not existing documentation to update.
- Tests added, just like `setup` and `teardown`.
pkg-pr-new[bot]: 
[Open in Stackblitz](https://pkg.pr.new/template/b8e8eeda-f8e7-41ba-927d-7f77ba8092f5)


```
npm i https://pkg.pr.new/tinylibs/tinybench@259
```


_commit: <a href="https://github.com/tinylibs/tinybench/runs/38928816907"><code>46d6e65</code></a>_

crimx: I don't see any breaking change? The callbacks are backward compatible. It is unlikely that user passed in a callback with an optional parameter.
jerome-benoit: 
> * I noticed the JSDoc artifacts are in the source code. Should I build the JSDoc manually? That would be hard to review.
Since the existing code is not documented, only the public API change should have a JSDoc, so the `FnHook` type definition should have a typedoc compatible comment. 
I will probably add a proper one on `Hook` type definition. 
> * There is not existing documentation to update.
I guess a properly commented `FnHook` type definition should be enough. 



jerome-benoit: > I don't see any breaking change? The callbacks are backward compatible. It is unlikely that user passed in a callback with an optional parameter.

See https://github.com/tinylibs/tinybench/pull/259#discussion_r1999878649
crimx: > > I don't see any breaking change? The callbacks are backward compatible. It is unlikely that user passed in a callback with an optional parameter.
> 
> See [#259 (comment)](https://github.com/tinylibs/tinybench/pull/259#discussion_r1999878649)

You don't need to make `mode` optional. Types added in callbacks are backward compatible. [Playground Link](https://www.typescriptlang.org/play/?#code/MYGwhgzhAEAqkGtoG8BQqC+6CWA7ALgKYBOAZmMIdAPYgAmA8gA74xrQfQBGhp1xhAIIgQACnwALbBABccRAEo5AN2rY6mHARLlK0XIQDuzVilSduvfkJHips+RAQAaaAFtqdQnIDkxAK64PtAAPtA+hmDEbv5MPkrQquqaqKSBwPjY1Lg09IJ0dKLULA60jCUKZilpuBlZOQaG+YXFrHKNJhCVaCllzaLsnDx8AsJi3egWWBgK6I39gxzD1mOiE+ac0wpAA)
jerome-benoit: > > See [#259 (comment)](https://github.com/tinylibs/tinybench/pull/259#discussion_r1999878649)
> 
> You don't need to make `mode` optional. Parameters added in callbacks are backward compatible. [Playground Link](https://www.typescriptlang.org/play/?#code/MYGwhgzhAEAqkGtoG8BQqC+6CWA7ALgKYBOAZmMIdAPYgAmA8gA74xrQfQBGhp1xhAIIgQACnwALbBABccRAEo5AN2rY6mHARLlK0XIQDuzVilSduvfkJHips+RAQAaaAFtqdQnIDkxAK64PtAAPtA+hmDEbv5MPkrQquqaqKSBwPjY1Lg09IJ0dKLULA60jCUKZilpuBlZOQaG+YXFrHKNJhCVaCllzaLsnDx8AsJi3egWWBgK6I39gxzD1mOiE+ac0wpAA)

It will fail with strict type checking options enabled. 

crimx: <img width="394" alt="image" src="https://github.com/user-attachments/assets/3e923b7c-8561-4eb6-8034-a2876573df43" />

No I believe it won't. The type would be broken in these two scenarios:

1. If users directly utilize the `FnHook` type for their definitions, but this type wasn't previously available, so it won't occur.
2. If users provide callbacks with optional parameters, which is unlikely given the context of how opts is used.

Patch:
diff --git a/src/task.ts b/src/task.ts
index fc50117..fee5297 100644
--- a/src/task.ts
+++ b/src/task.ts
@@ -107,6 +107,7 @@ export class Task extends EventTarget {
     this.dispatchEvent(createBenchEvent('start', this))
     await this.bench.opts.setup?.(this, 'run')
     const { error, samples: latencySamples } = (await this.benchmark(
+      'run',
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
       this.bench.opts.time!,
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
@@ -142,6 +143,7 @@ export class Task extends EventTarget {
     )
 
     const { error, samples: latencySamples } = this.benchmarkSync(
+      'run',
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
       this.bench.opts.time!,
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
@@ -170,6 +172,7 @@ export class Task extends EventTarget {
     this.dispatchEvent(createBenchEvent('warmup', this))
     await this.bench.opts.setup?.(this, 'warmup')
     const { error } = (await this.benchmark(
+      'warmup',
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
       this.bench.opts.warmupTime!,
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
@@ -198,6 +201,7 @@ export class Task extends EventTarget {
     )
 
     const { error } = this.benchmarkSync(
+      'warmup',
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
       this.bench.opts.warmupTime!,
       // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
@@ -214,12 +218,13 @@ export class Task extends EventTarget {
   }
 
   private async benchmark (
+    mode: 'run' | 'warmup',
     time: number,
     iterations: number
   ): Promise<{ error?: unknown; samples?: number[] }> {
     if (this.fnOpts.beforeAll != null) {
       try {
-        await this.fnOpts.beforeAll.call(this)
+        await this.fnOpts.beforeAll.call(this, mode)
       } catch (error) {
         return { error }
       }
@@ -230,7 +235,7 @@ export class Task extends EventTarget {
     const samples: number[] = []
     const benchmarkTask = async () => {
       if (this.fnOpts.beforeEach != null) {
-        await this.fnOpts.beforeEach.call(this)
+        await this.fnOpts.beforeEach.call(this, mode)
       }
 
       let taskTime = 0 // ms;
@@ -254,7 +259,7 @@ export class Task extends EventTarget {
       totalTime += taskTime
 
       if (this.fnOpts.afterEach != null) {
-        await this.fnOpts.afterEach.call(this)
+        await this.fnOpts.afterEach.call(this, mode)
       }
     }
 
@@ -282,7 +287,7 @@ export class Task extends EventTarget {
 
     if (this.fnOpts.afterAll != null) {
       try {
-        await this.fnOpts.afterAll.call(this)
+        await this.fnOpts.afterAll.call(this, mode)
       } catch (error) {
         return { error }
       }
@@ -291,12 +296,13 @@ export class Task extends EventTarget {
   }
 
   private benchmarkSync (
+    mode: 'run' | 'warmup',
     time: number,
     iterations: number
   ): { error?: unknown; samples?: number[] } {
     if (this.fnOpts.beforeAll != null) {
       try {
-        const beforeAllResult = this.fnOpts.beforeAll.call(this)
+        const beforeAllResult = this.fnOpts.beforeAll.call(this, mode)
         invariant(
           !isPromiseLike(beforeAllResult),
           '`beforeAll` function must be sync when using `runSync()`'
@@ -311,7 +317,7 @@ export class Task extends EventTarget {
     const samples: number[] = []
     const benchmarkTask = () => {
       if (this.fnOpts.beforeEach != null) {
-        const beforeEachResult = this.fnOpts.beforeEach.call(this)
+        const beforeEachResult = this.fnOpts.beforeEach.call(this, mode)
         invariant(
           !isPromiseLike(beforeEachResult),
           '`beforeEach` function must be sync when using `runSync()`'
@@ -336,7 +342,7 @@ export class Task extends EventTarget {
       totalTime += taskTime
 
       if (this.fnOpts.afterEach != null) {
-        const afterEachResult = this.fnOpts.afterEach.call(this)
+        const afterEachResult = this.fnOpts.afterEach.call(this, mode)
         invariant(
           !isPromiseLike(afterEachResult),
           '`afterEach` function must be sync when using `runSync()`'
@@ -358,7 +364,7 @@ export class Task extends EventTarget {
 
     if (this.fnOpts.afterAll != null) {
       try {
-        const afterAllResult = this.fnOpts.afterAll.call(this)
+        const afterAllResult = this.fnOpts.afterAll.call(this, mode)
         invariant(
           !isPromiseLike(afterAllResult),
           '`afterAll` function must be sync when using `runSync()`'
diff --git a/src/types.ts b/src/types.ts
index c476bbb..41212fa 100644
--- a/src/types.ts
+++ b/src/types.ts
@@ -113,6 +113,13 @@ export type EventListener = (evt: BenchEvent) => void
 // eslint-disable-next-line @typescript-eslint/no-redundant-type-constituents
 export type Fn = () => Promise<unknown> | unknown
 
+/**
+ * The task hook function signature
+ * If warmup is enabled, the hook will be called twice, once for the warmup and once for the run.
+ * @param mode the mode where the hook is being called
+ */
+export type FnHook = (this: Task, mode?: 'run' | 'warmup') => Promise<void> | void
+
 /**
  * the task function options
  */
@@ -120,26 +127,27 @@ export interface FnOptions {
   /**
    * An optional function that is run after all iterations of this task end
    */
-  afterAll?: (this: Task) => Promise<void> | void
+  afterAll?: FnHook
 
   /**
    * An optional function that is run after each iteration of this task
    */
-  afterEach?: (this: Task) => Promise<void> | void
+  afterEach?: FnHook
 
   /**
    * An optional function that is run before iterations of this task begin
    */
-  beforeAll?: (this: Task) => Promise<void> | void
+  beforeAll?: FnHook
 
   /**
    * An optional function that is run before each iteration of this task
    */
-  beforeEach?: (this: Task) => Promise<void> | void
+  beforeEach?: FnHook
 }
 
 /**
  * Hook function signature
+ * If warmup is enabled, the hook will be called twice, once for the warmup and once for the run.
  * @param task the task instance
  * @param mode the mode where the hook is being called
  */


Test patch:
diff --git a/test/index.test.ts b/test/index.test.ts
index 7a68171..e1b8fb0 100644
--- a/test/index.test.ts
+++ b/test/index.test.ts
@@ -990,11 +990,13 @@ test('task beforeAll, afterAll, beforeEach, afterEach (async)', async () => {
   await bench.run()
 
   expect(beforeAll).toHaveBeenCalledTimes(2 /* warmup + run */)
+  expect(beforeAll.mock.calls).toEqual([['warmup'], ['run']])
   expect(afterAll).toHaveBeenCalledTimes(2 /* warmup + run */)
-  expect(beforeAll.mock.calls.length).toBe(afterAll.mock.calls.length)
+  expect(afterAll.mock.calls).toEqual([['warmup'], ['run']])
   expect(beforeEach).toHaveBeenCalledTimes(iterations * 2 /* warmup + run */)
+  expect(beforeEach.mock.calls).toEqual(Array(iterations).fill(['warmup']).concat(Array(iterations).fill(['run'])))
   expect(afterEach).toHaveBeenCalledTimes(iterations * 2 /* warmup + run */)
-  expect(beforeEach.mock.calls.length).toBe(afterEach.mock.calls.length)
+  expect(afterEach.mock.calls).toEqual(Array(iterations).fill(['warmup']).concat(Array(iterations).fill(['run'])))
 })
 
 test('task beforeAll, afterAll, beforeEach, afterEach (sync)', () => {
@@ -1034,11 +1036,13 @@ test('task beforeAll, afterAll, beforeEach, afterEach (sync)', () => {
   bench.runSync()
 
   expect(beforeAll).toHaveBeenCalledTimes(2 /* warmup + run */)
+  expect(beforeAll.mock.calls).toEqual([['warmup'], ['run']])
   expect(afterAll).toHaveBeenCalledTimes(2 /* warmup + run */)
-  expect(beforeAll.mock.calls.length).toBe(afterAll.mock.calls.length)
+  expect(afterAll.mock.calls).toEqual([['warmup'], ['run']])
   expect(beforeEach).toHaveBeenCalledTimes(iterations * 2 /* warmup + run */)
+  expect(beforeEach.mock.calls).toEqual(Array(iterations).fill(['warmup']).concat(Array(iterations).fill(['run'])))
   expect(afterEach).toHaveBeenCalledTimes(iterations * 2 /* warmup + run */)
-  expect(beforeEach.mock.calls.length).toBe(afterEach.mock.calls.length)
+  expect(afterEach.mock.calls).toEqual(Array(iterations).fill(['warmup']).concat(Array(iterations).fill(['run'])))
 })
 
 test(