feat(generator): create a toolbox #8295

fbeauchamp · 2025-01-31T10:25:44Z

Description

Code written with the goal of being compatible with node 23.6+, no enum nor protected

Review in order :

throttle : throttle a generator to limit its speed.
timeout : emit an error if a generator fail to answer in time
synchronized :
- fork a generator into multiple forks.
- All the forks are yielding in sync, to keep memory pressure under control.
- If the source fais all the forks fails
- if a forks ends or fail prematurely, it is removed from the fork list
- if the last fork ends or fails it's also stopping the source generator

Checklist

Commit
- Title follows commit conventions
- Reference the relevant issue (Fixes #007, See xoa-support#42, See https://...)
- If bug fix, add Introduced by
Changelog
- If visible by XOA users, add changelog entry
- Update "Packages to release" in CHANGELOG.unreleased.md
PR
- If UI changes, add screenshots
- If not finished or not tested, open as Draft

Review process

This 2-passes review process aims to:

develop skills of junior reviewers

limit the workload for senior reviewers

limit the number of unnecessary changes by the author

The author creates a PR.
Review process:
1. The author assigns the junior reviewer.
2. The junior reviewer conducts their review:
  - Resolves their comments if they are addressed.
  - Adds comments if necessary or approves the PR.
3. The junior reviewer assigns the senior reviewer.
4. The senior reviewer conducts their review:
  - If there are no unresolved comments on the PR → merge.
  - Otherwise, we continue with 3.
The author responds to comments and/or makes corrections, and we go back to 2.

Notes:

The author can request a review at any time, even if the PR is still a Draft.
In theory, there should not be more than one reviewer at a time.
The author should not make any changes:
- When a reviewer is assigned.
- Between the junior and senior reviews.

@vates/generator-toolbox/package.json

stephane-m-dev · 2025-02-03T22:59:15Z

@vates/generator-toolbox/src/synchronized.mts

+      const next = this.#source.next().catch(async error => {
+        const e = new Error(`Error in the source generator ${error.message}`)
+        // @todo : why  can't I set the cause ?
+        //         e.cause = error


Don't let commented code if you don't have a solution at the moment

do you know of can I set the cause of an error in typescript ? this looks like a standard property for me https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

throw new Error("Action failed.", { cause: err }); ?

Expected 0-1 arguments, but got 2

it may be my IDE that is misconfigured

stephane-m-dev · 2025-02-03T23:05:37Z

@vates/generator-toolbox/src/throttle.mts

+    } else {
+      speed = this.#bytesPerSecond
+    }
+    assert.ok(speed > 0, `speed must be greate than zero, ${speed} computed`)


great -> greater

stephane-m-dev · 2025-02-03T23:08:43Z

@vates/generator-toolbox/src/throttle.mts

+    assert.ok(speed > 0, `speed must be greate than zero, ${speed} computed`)
+    return speed
+  }
+  constructor(speed: number | (() => number)) {


I think you should set a default speed or force minimal speed, as speed must be greater than zero.

a positive speed is already enforced through assert. How would you do it ?

speed: number = 1234

stephane-m-dev · 2025-02-03T23:16:16Z

@vates/generator-toolbox/src/throttle.mts

+        yield value
+      }
+    } finally {
+      // in case of error : cleanup the timeout


In finally it will happen in case or error or success.
I think you could completely remove this comment, it doesn't bring anything to comment the clearTimeout() just after.

stephane-m-dev · 2025-02-03T23:19:53Z

@vates/generator-toolbox/src/throttle.test.mts

+    assert.strictEqual(Math.round((end - start) / 1000), 2)
+  })
+  test('multiple generator test', async () => {
+    const generator = makeGenerator(1024 * 1024, 10)


Couldn't you put it in a beforeEach() ?

stephane-m-dev · 2025-02-03T23:21:41Z

@vates/generator-toolbox/src/timeout.mts

+export class Timeout<T> implements AsyncGenerator {
+  #source: AsyncGenerator<T>
+  #timeout: number
+  constructor(source, timeout: number) {


I think you should set a default timeout value or allow min/max range.

I think timeout value is very context dependent, and not setting one should force the user to use something sane in his context
I could force it to be a positive number though

MathieuRA · 2025-02-04T12:13:43Z

@vates/generator-toolbox/package.json

+{
+  "name": "@vates/generator-toolbox",
+  "version": "0.0.0",
+  "main": "src/index.mts",


Is not supposed to be dist/index.mjs?

MathieuRA · 2025-02-04T12:17:40Z

@vates/generator-toolbox/package.json

+  "version": "0.0.0",
+  "main": "src/index.mts",
+  "license": "MIT",
+  "private": true,


If you set "private": true in your package.json, then npm will refuse to publish it.
https://docs.npmjs.com/cli/v11/configuring-npm/package-json#private

Why?

I want to be sure this is not published for now . But I will have to remove this flag before merge

MathieuRA · 2025-02-04T12:20:09Z

@vates/generator-toolbox/package.json

+    "@tsconfig/node-lts": "^20.1.3",
+    "@tsconfig/recommended": "^1.0.7",


If you add theses dependencies, then you need to extends your tsconfig.json
See: https://www.npmjs.com/package/@tsconfig/recommended

Are these packages really necessary?

I tryed to follow : https://typescript-eslint.io/getting-started/

MathieuRA · 2025-02-04T12:21:29Z

@vates/generator-toolbox/package.json

+    "url": "https://vates.fr"
+  },
+  "engines": {
+    "node": ">=8.10"


Maybe node: >=22.3.0?
To match your @types/node version. No?

CI is still on 20.x

eslint didn't block this incomplete commit

julien-f reviewed Jan 31, 2025

View reviewed changes

@vates/generator-toolbox/package.json Outdated Show resolved Hide resolved

fbeauchamp force-pushed the feat_generator_toolbox branch 2 times, most recently from e040772 to e1f9db6 Compare February 2, 2025 15:10

fbeauchamp marked this pull request as ready for review February 3, 2025 07:06

fbeauchamp requested a review from stephane-m-dev February 3, 2025 07:33

fbeauchamp added 8 commits February 3, 2025 12:09

feat(generator): implements throttler and synchronized

0a69d05

deepkseek review

ad510b5

feat(generator/throttler): implement variable speed limits

9c5438c

fix(generator/synchronized): can't fork a started generator

5c7cb88

feat(generator): handle generator timeout

1a143df

chore(generator): tsconfig.json

7a1cb46

code cleanup

3c0c91e

rename typo in filename and enable tests on ci

9e38299

fbeauchamp force-pushed the feat_generator_toolbox branch from cd49201 to 6d281b3 Compare February 3, 2025 11:10

changelog

6e0a61b

fbeauchamp force-pushed the feat_generator_toolbox branch from 6d281b3 to 6e0a61b Compare February 3, 2025 11:12

yanrlock

54ee930

stephane-m-dev requested changes Feb 3, 2025

View reviewed changes

stephane-m-dev requested a review from MathieuRA February 4, 2025 08:46

fix following review

a0aa05d

MathieuRA requested changes Feb 4, 2025

View reviewed changes

MathieuRA self-requested a review February 4, 2025 12:24

fbeauchamp added 5 commits February 4, 2025 13:56

wip eslint

81e7041

eslint didn't block this incomplete commit

missing file

e8569e3

eslint fixes

c2b68ce

downgrade node to please the CI

e308078

put back tsc build

e501738

fbeauchamp force-pushed the feat_generator_toolbox branch from 4fdcfb0 to 90ebbc5 Compare February 5, 2025 13:19

more tests on ci

af16bee

fbeauchamp force-pushed the feat_generator_toolbox branch from 90ebbc5 to af16bee Compare February 5, 2025 13:25

fbeauchamp added 2 commits February 5, 2025 13:31

build before tests

dae1520

more flaky test

4850e6d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(generator): create a toolbox #8295

feat(generator): create a toolbox #8295

fbeauchamp commented Jan 31, 2025 •

edited

Loading

stephane-m-dev Feb 3, 2025

fbeauchamp Feb 4, 2025

stephane-m-dev Feb 4, 2025

fbeauchamp Feb 4, 2025

fbeauchamp Feb 4, 2025

stephane-m-dev Feb 3, 2025

stephane-m-dev Feb 3, 2025

fbeauchamp Feb 4, 2025

stephane-m-dev Feb 4, 2025

stephane-m-dev Feb 3, 2025 •

edited

Loading

stephane-m-dev Feb 3, 2025

stephane-m-dev Feb 3, 2025

fbeauchamp Feb 4, 2025

MathieuRA Feb 4, 2025

MathieuRA Feb 4, 2025

fbeauchamp Feb 5, 2025

MathieuRA Feb 4, 2025

fbeauchamp Feb 5, 2025

MathieuRA Feb 4, 2025

fbeauchamp Feb 5, 2025

		"@tsconfig/node-lts": "^20.1.3",
		"@tsconfig/recommended": "^1.0.7",

feat(generator): create a toolbox #8295

Are you sure you want to change the base?

feat(generator): create a toolbox #8295

Conversation

fbeauchamp commented Jan 31, 2025 • edited Loading

Description

Checklist

Review process

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephane-m-dev Feb 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fbeauchamp commented Jan 31, 2025 •

edited

Loading

stephane-m-dev Feb 3, 2025 •

edited

Loading