Replace ESM-only archive and port dependencies#1370
Conversation
cristianrgreco
added
dependencies
✅ Deploy Preview for testcontainers-node ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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: e23a2df7a8
ℹ️ 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".
cristianrgreco
force-pushed
the
codex/remove-esm-only-deps
branch
from
e23a2df to
3a0853f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a0853f7e3
ℹ️ 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".
| }; | ||
|
|
||
| const createContentArchiveEntry = async (content: Content, target: string, mode?: number): Promise<TarArchiveEntry> => { | ||
| const buffer = await toBuffer(content); |
There was a problem hiding this comment.
When a caller passes a Readable to withCopyContentToContainer or copyContentToContainer, this awaits toBuffer(content) before the archive is handed to Docker, so the entire stream must fit in memory and upload cannot begin until it has fully drained. The previous archiver.append path accepted streams directly, so large generated payloads that used to stream through can now exhaust heap; keep stream entries streaming or otherwise avoid materializing the full content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I checked the previous archiver tar path before changing this. archiver.append(Readable) only streamed directly when stats/size were provided; for a Readable without stats it used collectStream(...) and buffered the stream before writing the tar entry. ContentToCopy does not currently carry size metadata, and tar-stream needs size before writing the header, so this PR preserves the existing behavior rather than introducing a new API. We can add optional size-aware streaming separately if we want to support that.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9019356c85
ℹ️ 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".
| await new Promise<void>((resolve, reject) => { | ||
| server.close((err) => (err ? reject(err) : resolve())); | ||
| }); | ||
|
|
||
| return address.port; |
There was a problem hiding this comment.
After finding a free port, this closes the probe socket and immediately returns the number without recording it. In the common case where the same process asks for several ports before Docker actually binds them (for example parallel container starts or tests that allocate several fixed host ports), a later call can receive the same released port; the old get-port path kept returned ports out of the candidate set temporarily, so those in-process collisions were avoided. Add a short-lived/reserved set or keep the socket reserved until the caller binds it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eefa0960ec
ℹ️ 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".
| header: { | ||
| name: normalizeTarPath(target), | ||
| type: "symlink", | ||
| linkname: await fs.readlink(source), |
There was a problem hiding this comment.
When a copied directory contains an absolute symlink to another file inside the same source tree (for example link -> /host/src/file), this stores the host path verbatim as the tar link target. Docker then recreates an absolute link in the container pointing at /host/src/file, while the file itself was copied under the requested container target, so the symlink is broken; the previous archiver.directory path normalized symlink targets relative to the symlink location. Convert absolute symlink targets that resolve inside the copied tree to the corresponding target-relative linkname before archiving.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4e29c7dd34
ℹ️ 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".
| header: { | ||
| name: normalizeTarPath(target), | ||
| type: "directory", | ||
| mode: getEntryMode(stats, mode), |
There was a problem hiding this comment.
On Windows hosts, copied subdirectories are archived with the raw fs.lstat mode when the caller doesn't pass mode. Windows commonly reports directory permissions as 0666 with no execute bits, while the previous archiver path normalized directory entries to traversable modes; extracting this tar into a Linux container can leave nested copied directories inaccessible, so files under them cannot be reached unless every directory copy supplies an explicit mode. Default directory entries to executable permissions on win32, e.g. the old 0755 behavior, before writing the header.
Useful? React with 👍 / 👎.
|
As we can see from the PR feedback, trying to replace archiver with tar-stream is more complex than we'd like. As of now I'm not comfortable owning so much low-level tar code. Closing for now |
1 participant
Summary
archiverusage with an internaltar-streamarchive builder for copy-to-container paths.get-portusage with a nativenet.listen(0)implementation exposed asgetRandomPort().archiverandget-port, plus related test-only uses, while adding focused tar archive tests.Verification
npm run formatnpm run lintnpm run check-compilesnpm test -- --run packages/testcontainers/src/utils/port-generator.test.tsnpm test -- --run packages/testcontainers/src/utils/tar-archive.test.tsnpm test -- --run packages/testcontainers/src/generic-container/generic-container.test.ts -t "bind to specified host port|copy"npm test -- --run packages/modules/couchbase/src/couchbase-container.test.ts -t "preserve fixed host port binding"packages/testcontainers/build/index.jsforgetRandomPortTest Results
Semver Impact
Patch. This removes direct ESM-only runtime dependencies while preserving the existing CommonJS package shape and public copy APIs.
RandomPortGeneratorremains exported for compatibility;getRandomPort()is additive. Existing copy-to-container behavior is covered by Docker-backed tests, including file, directory, content, permission, and archive ownership paths.The direct consumer install footprint drops by about 3.8 MiB in a clean production install comparison.