Update deps and tooling to reduce installation footprint - #218
Conversation
|
@benoit74 just a friendly bump on this to take a look at your convenience |
benoit74
left a comment
There was a problem hiding this comment.
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.
|
@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 ^^ |
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.
Noted!
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 😅 |
|
@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 😄 Can port it to a proper PR for this repo when either:
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 |
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
left a comment
There was a problem hiding this comment.
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. |
This is fine |
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/fakerstill 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):
Before (107 dependencies):
After (25 dependencies):