Skip to content

Update deps and tooling to reduce installation footprint - #218

Merged
achou11 merged 8 commits into
openzim:mainfrom
achou11:ac/reduce-publish-footprint
Aug 3, 2026
Merged

Update deps and tooling to reduce installation footprint#218
achou11 merged 8 commits into
openzim:mainfrom
achou11:ac/reduce-publish-footprint

Conversation

@achou11

@achou11 achou11 commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

This PR brings in quite a few changes and I fully acknowledge that there may be some reversions or adjustments based on maintainers' preferences.

  • Moves some of the still-necessary dependencies as dev-only, which prevents unnecessary installation from consuming projects. Basically @types/* modules and things used only in the tests.

  • Leans into Node's support for TypeScript files via type-stripping, which is primarily relevant for the tests that are written in TypeScript. This removes the need for something like ts-node.

  • Updates the TypeScript configuration to align with Node-based projects, most notably related to the native TS support and a stricter adherence to Node-oriented ESM module syntax (e.g. requiring file extension names in imports).

  • Replaces Jest with Node's built-in test runner. This means updates to the npm scripts that run the tests as well as minor updates to the tests themselves. Note that as of now, I have not made use of the built-in assertions module. Instead, I installed expect, which is Jest's standalone module for its assertions API. This avoids needing to completely rewrite the tests for now.

Some open questions:

  • Is there any interest in moving away from Jest's expect API and instead using Node's built-in assertions for the tests? This would further reduce the dependency usage for this project (although it doesn't affect consuming projects). Porting the tests would be pretty straightforward from what I can tell.

  • Is @faker-js/faker still actually used in practice? I noticed that there's some commented-out lines in the memory-leak tests that would make use of it. Not sure if those are still relevant or not but if not, could remove that dependency as well.


Deps graph before and after this PR (when installing as consumer):

Comment thread dev/hackPackageVersion.js Outdated
Comment thread .vscode/launch.json Outdated
Comment thread package.json
@achou11

achou11 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

@benoit74 just a friendly bump on this to take a look at your convenience

@benoit74
benoit74 self-requested a review July 31, 2026 14:15

@benoit74 benoit74 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please remove @faker-js/faker dependency if unused actually. Always easy to add back once we know why we need it.

Please also remove commented-out memory leak tests, there is no point in keeping them.

Regarding the fact that we move away from expect, I don't know exactly what it entails. Probably worth to get it a (small) try to confirm if code looks more readable or if there is a concern. Like migrate few tests in a subsequent PR, push it as draft so that I can review and we agree if we continue the effort to completely remove expect or if it is not worth it.

@benoit74

Copy link
Copy Markdown
Contributor

@achou11 please ping me (or ask for review if you can) once PR is ready, otherwise I do not know if it is still a WIP or not. People should use "draft" attribute for that ... but barely no-one does ... No worries, but this explains why I did not reviewed it (and I also forgot a bit tbh).

I've invited you on the project so that you can stop working from your fork, should help as well ^^

@achou11

achou11 commented Jul 31, 2026

Copy link
Copy Markdown
Collaborator Author

Regarding the fact that we move away from expect, I don't know exactly what it entails. Probably worth to get it a (small) try to confirm if code looks more readable or if there is a concern. Like migrate few tests in a subsequent PR, push it as draft so that I can review and we agree if we continue the effort to completely remove expect or if it is not worth it.

Makes sense. Also worth noting that Chai provides an similar expect API but has a much smaller install footprint, so that could also be a good option to consider (believe Vitest technically uses this under the hood too).

https://www.chaijs.com/api/bdd/

Thinking I'll do a sample using both node's built-in assert and Chai.

please ping me (or ask for review if you can) once PR is ready, otherwise I do not know if it is still a WIP or not. People should use "draft" attribute for that ... but barely no-one does ... No worries, but this explains why I did not reviewed it (and I also forgot a bit tbh).

Noted!

I've invited you on the project so that you can stop working from your fork, should help as well

Much appreciated! Not sure if anyone else from your org/repo had input on this, but would be helpful for me to know that others are also okay with me joining this repo. Kind of a bit odd for me to advocate as the one being invited, but just want to encourage safe practices when it comes to access control, especially given the current climate 😅

@achou11

achou11 commented Aug 1, 2026

Copy link
Copy Markdown
Collaborator Author

@benoit74 this PR is ready for re-review!

For the expect-to-assert exploration, I have a stacked draft PR in my fork to more easily view the diff since I didn't want to accept your invitation until my question was answered 😄

achou11#1

Can port it to a proper PR for this repo when either:

  • I end up accepting the invite
  • This PR is merged

Thinking I'll do a sample using both node's built-in assert and Chai.

I decided not to do a Chai exploration for now because the expect API out of the box is actually different than I thought, which conflicted with a typescript-eslint rule that I didn't want to mess with. Getting the API that's aligned requires extending Chai using @vitest/expect. Can still explore if interested but will hold off for now.

@benoit74

benoit74 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Much appreciated! Not sure if anyone else from your org/repo had input on this, but would be helpful for me to know that others are also okay with me joining this repo. Kind of a bit odd for me to advocate as the one being invited, but just want to encourage safe practices when it comes to access control, especially given the current climate 😅

This invite matches our "policy" on new contributors at Kiwix / openZIM level. I do agree we could probably have safer practices, but at the same time the "Write" permissions is not a full control of the repo and it is currently the strict minimum role available on Github to have smooth daily operations.

@benoit74 benoit74 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM. Do you want to squash some commits or do they all makes sense in the future and we shall merge as-is?

@achou11

achou11 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator Author

LGTM. Do you want to squash some commits or do they all makes sense in the future and we shall merge as-is?

I usually use the "Squash and merge" option when merging PRs, but can squash beforehand if it's preferable.

@benoit74

benoit74 commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

I usually use the "Squash and merge" option when merging PRs, but can squash beforehand if it's preferable.

This is fine

@achou11
achou11 merged commit cfa34db into openzim:main Aug 3, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants