refactor: replace archiver with modern-tar - #1427
Conversation
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
ayuhito
changed the title
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48e7f96f26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Ah, I didn't know modern-tar was ESM only 🫣 In that case this would require a major release, but I'd need to see if there are workarounds for Jest users. Although with dynamic imports we won't encounter issues at load time like in #1365, any usage of modern-tar would still fail |
|
I notice modern-tar has no deps, and uses tsdown for its build. Would it be possible / is it something you would want to do, to enable dual-publishing? I think it would be a config addition via tsdown, and shouldn't require any changes for existing users, but would add CJS compatibility. It would however increase your bundle size |
|
Oops, yeah I didn't think that through. This PR can't be merged as is.
In principle, I don't want to support CJS as a way to push the ecosystem forward and also the added package bloat. Sorry for being a bit uncooperative on this matter. Jest is one of the unfortunate incumbents 😔
But, this actually is a really interesting idea if we were to apply it to |
No problem at all, I agree it would take away a bit the "modern" part of modern-tar 🙂
It is interesting. It'd solve such future problems as well. For example we're currently stuck on an old version of get-port for the same reason. There are downsides, e.g. our bundle size will increase, clients won't dedupe dependencies, etc. But for small libs, especially with no deps like modern-tar and get-port, it may be a good compromise. I will need time to look into it. A first approach may be to bundle the latest get-port and get that released. If it's all good then we could add modern-tar to the list in this PR and that'd be it |
|
Feel free to close this PR if needed and handle things which you think is best! Happy to help in the future whenever 👍 |
|
Thanks @ayuhito. Btw what do you think if I were to create a fork of modern-tar which repackages modern-tar as CJS, and published it to a lib called I am slightly hesitant to replace Testcontainer's build system, so wondering if managing forks is a better alternative |
It would work but I fear it isn't an ideal precedent to set, nor would be a good burden to maintain.
But I can understand this and the motivation for a fork. I was curious what the The diff is mostly Not sure if this is the direction you want to go, but it does seem quite feasible? |
2 participants
Overview
Related #1161 #1410
This replaces
archiverwithmodern-tar. Nets in a reduced dependency surface and some performance improvements for related workloads (although most likely not noticeable).There are likely some very low level differences between these libraries when it comes to error or symlink handling, but I would like to believe
modern-tarfollows more correct behaviour.archiverdoes a lot of non-standard things based on years of changes.Notes
Additionally,
modern-taris an ESM-only package, however, with dynamic imports we should be covered against cases like #1365.I'll also make follow-up PRs to replace some instances of
tar-fsin the codebase. Unfortunately, it's not possible to create stacked PRs from forks so that will happen after this PR is merged 📦