perf: replace archiver and tar-fs with modern-tar #1161
Conversation
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| expect((await container.exec(`stat -c "%a %n" /tmp/newdir/test.txt`)).output).toContain("777"); | ||
| // Check directory permissions | ||
| expect((await container.exec(`stat -c "%a %n" /tmp/newdir`)).output).toContain("777"); | ||
|
|
||
| // Verify files retain their original permissions | ||
| expect((await container.exec(`stat -c "%a %n" /tmp/newdir/test.txt`)).output).toContain("644"); |
There was a problem hiding this comment.
I think this is a bug/edge case from archiver, because node-tar and modern-tar do not mix directory modes and file modes together. That's not very standard behaviour.
| const data = content instanceof Readable ? await buffer(content) : content; | ||
| return { type: "content" as const, content: data, target, mode }; |
There was a problem hiding this comment.
archiver implicitly buffers Readable. We're explicit here (and also maybe more efficient since we use Node native buffering).
|
I tried to validate as many tests as possible locally, but obviously this suite is very heavy so CI might fail if I didn't cover everything once these workflows are approved 🙈 |
cristianrgreco
added
enhancement
| // Can be removed if testcontainers switches to ESM. | ||
| declare module "modern-tar/fs" { | ||
| export * from "modern-tar/dist/fs/index"; | ||
| } |
There was a problem hiding this comment.
module: "commonjs" doesn't really support ESM only packages which use an export map. It can just read the old main import instead which is our browser specific export.
Alternatively, we could modernize the whole tsconfig stack. Then you could still export to CJS via something like tsdown. But this is the least-intrusive approach I can think of for now.
There was a problem hiding this comment.
We already have a few dependencies which are ESM only, such as get-port, and it is enough to do an await import("get-port"). If modern-tar produces an export map then things should just work with TS without needing to add declarations
|
Hi @ayuhito, thanks for raising an excellent PR! Although I accept that the current dependency |
|
Hi @cristianrgreco, that is very reasonable and I can understand the tentativeness behind this change. The reason I created this new package in the first place was that there was no reasonable lightweight alternative, but it definitely needs time to mature in the ecosystem. Although I would say I've done extensive work hardening the project and matching |
Definitely! Thanks again for raising the PR, hopefully will collaborate again soon 🙂 |
2 participants
Overview
Resolves #1135 using
modern-tar. Around a 48MB --> 38MB reduction for package size which adds up a lot when considering download count. We're probably also a bit faster.The next most impactful thing could be to reduce dependencies in
dockerode, but they seem to be targetingnode@8so upping their limit to at leastnode@18could help us start removing dependencies there.Ref: https://npmgraph.js.org/?q=testcontainers
Please let me know if there are any concerns! I'm happy to look into it all!