fix(js-sdk): use commands.run in Sandbox.getHost() example#1531
Open
anxkhn wants to merge 11 commits into
Open
fix(js-sdk): use commands.run in Sandbox.getHost() example#1531anxkhn wants to merge 11 commits into
anxkhn wants to merge 11 commits into
Conversation
anxkhn
requested review from
ValentaTomas,
jakubno and
mishushakov
as code owners
🦋 Changeset detectedLatest commit: b2cf887 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![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: 61d2c5620f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| * const sandbox = await Sandbox.create() | ||
| * // Start an HTTP server | ||
| * await sandbox.commands.exec('python3 -m http.server 3000') | ||
| * await sandbox.commands.run('python3 -m http.server 3000') |
There was a problem hiding this comment.
When users copy-paste this getHost() example, commands.run still does not reach the getHost line because Commands.run waits for completion unless opts.background is true (packages/js-sdk/src/sandbox/commands/index.ts lines 408-423). python3 -m http.server is a long-lived process, so this await will hang or time out instead of starting a server and returning a host; the existing HTTP-server tests use { background: true } for this case.
Useful? React with 👍 / 👎.
Member
|
@anxkhn please fix the codex comment and in any other places |
## What Modernizes the JS SDK's dependencies while remaining fully compatible with the current supported Node range (`>=20.18.1`) — no engine changes and no breaking impact for consumers. - **`@connectrpc/connect` / `@connectrpc/connect-web`:** `2.0.0-rc.3` → `^2.1.2` (off the pre-release pin onto the stable line, and switched to a `^` range). - **`@bufbuild/protobuf`:** `^2.6.2` → `^2.12.1`. - **React test deps:** `react` / `@types/react` → `^19.2.0`, and `react-dom` / `@types/react-dom` added at `^19.2.0` (previously auto-installed as v18 peers). Dev/test-only — no runtime impact. - **CI:** standardized `actions/setup-node` (mixed v3/v4/v6) to `v6` across all workflows; the three `@v3` uses were on the deprecated Node16 action runtime. No public SDK API changes — the sandbox filesystem and command RPCs use the same Connect transport configuration. ## Why undici / Node floor were dropped from this PR An earlier revision also bumped `undici` 7 → 8 and raised the Node floor to `>=22.19.0`. Usage data shows **Node 20 is still the single largest SDK runtime (~39% of sandbox creations)**, so dropping it would break the largest consumer segment via `engine-strict` install failures. undici 8 was the *only* change forcing Node 22, and undici `7.28.0` (already the latest 7.x) supports Node 20 — so undici stays at `^7.28.0` and the engine floor is unchanged. undici 8 is a good candidate for a future major once Node 20 usage declines. ## Verification - typecheck, lint (oxlint), and build pass - 22 mocked Connect/undici transport unit tests pass - 106 live filesystem/command tests pass over connectrpc `2.1.2` + undici `7.28.0` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Switches the build tooling for `packages/js-sdk` and `packages/cli` from `tsup` (esbuild) to `tsdown` (rolldown), replacing each `tsup.config.js` with a `tsdown.config.ts` and updating the `build`/`dev` scripts and devDependencies. The published artifact layout is intentionally unchanged — the SDK still ships `dist/index.js` (CJS), `dist/index.mjs` (ESM) and `dist/index.d.ts`/`.d.mts`, and the CLI still ships an executable `dist/index.js` plus `dist/templates` — kept identical via `fixedExtension: false`. CLI dependency bundling is preserved by mapping the old `noExternal` to tsdown's `deps.alwaysBundle` (still excluding the ESM-only, dynamically-imported `inquirer`), and template copying moves from an `onSuccess` shell step to tsdown's `copy` option. Also aligns Node versions: `engines.node` for both packages is set to `20 || >=22`, the CLI build targets `node20`, and the pinned `nodejs` in `.tool-versions` is bumped to `22.11.0`. The large `pnpm-lock.yaml` diff is expected — it swaps the tsup/esbuild dependency tree for tsdown's rolldown tree (no lockfile format change). ## Verification - Both packages build cleanly with output filenames identical to the previous tsup builds. - `typecheck`, `lint` (oxlint) and `build` pass for both packages; the built CLI runs (`--version`). - Built js-sdk imports correctly in both CJS (`require`) and ESM (`import`), exposing the default `Sandbox` export and all named exports. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adds UTM parameters to the e2b.dev link(s) in the README so GitHub-referral traffic is attributed per repo. `utm_source=github&utm_medium=referral&utm_campaign=readme&utm_content=<repo>` Applies to apex e2b.dev links (root and subpaths). Subdomains and already-tagged links are untouched, and URL anchors are preserved.
## Why The Playwright browser cache in `js_sdk_tests.yml` keyed on the Node version + a hash of `packages/js-sdk/package.json`. Node bumps (e.g. e2b-dev#1515) and release-bot version bumps rotated the key, so PRs kept re-downloading Chromium — ~3 minutes per Windows job, twice per run (staging + production) — e.g. [this run](https://github.com/e2b-dev/E2B/actions/runs/29036879724/job/86183938330?pr=1536). The churn also created a fresh ~250 MB cache entry per OS on every release. ## What Browser binaries depend only on the Playwright version, so the cache is now keyed on the installed Playwright version (read from `node_modules` after `pnpm install`), and the two OS-conditional cache steps are collapsed into one. The key only rotates when Playwright itself is upgraded, which is exactly when a re-download is needed. On a cache hit, the `pretest` `playwright install` becomes a no-op skip instead of a download. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…-side) (e2b-dev#1536) Supersedes e2b-dev#1516 (same modernization at TypeScript 6.0). Rebased onto `main` now that the build runs on **tsdown** (e2b-dev#1515). ## What & why Adopt **TypeScript 7** for both packages and modernize the compiler config. TypeScript 7.0's native compiler [ships no programmatic API yet](https://devblogs.microsoft.com/typescript/announcing-typescript-7-0/#running-side-by-side-with-typescript-6.0) (it lands in 7.1), so anything built on the TS compiler API breaks on it — here that's tsdown's `.d.ts` generation and the codegen scripts (`openapi-typescript`, `json-schema-to-typescript`). Per the official guidance, TS 7 is installed **side-by-side** with TS 6: ```json "@typescript/native": "npm:typescript@^7.0.2", // native tsc — used for type-checking "typescript": "npm:@typescript/typescript6@^6.0.2" // TS6 w/ compiler API — used by tooling ``` - `tsc --noEmit` (typecheck) → **native TypeScript 7.0.2** (verified: `tsc --version` → 7.0.2) - `import 'typescript'` → **TypeScript 6.0** *with* the compiler API → tsdown dts + codegen keep working - Bonus: tsdown's dts no longer prints the "TypeScript 7.0 does not yet have a stable API and is experimental" warning (it's on the 6.0 API now) **Internal build-config change only — no public API or runtime behavior changes.** ## Compiler options: before → after ### `packages/js-sdk/tsconfig.json` | option | before | after | |---|---|---| | `target` | `es6` | `es2022` | | `lib` | `["dom","ESNext"]` | `["dom","es2022"]` | | `module` | _(unset)_ | `esnext` | | `moduleResolution` | `node` | `bundler` | | `allowJs` | `true` | **removed** (no `.js` sources) | | `allowSyntheticDefaultImports` | `true` | **removed** (implied by `esModuleInterop`) | | `useDefineForClassFields` | _(false, implied by es6)_ | **`false` (now explicit)** — see note | ### `packages/cli/tsconfig.json` | option | before | after | |---|---|---| | `moduleResolution` | `node` | `bundler` | | `strictNullChecks`, `strictFunctionTypes`, `strictBindCallApply`, `strictPropertyInitialization`, `noImplicitThis`, `alwaysStrict` | `true` | **removed** (implied by `strict`) | | `downlevelIteration` | `true` | **removed** (removed in TS 7; no-op at `es2022`) | | `baseUrl` | `"."` | **removed** (removed in TS 7) | | `paths` | `{ e2b }` | `{ src, "src/*", e2b }` (replaces `baseUrl` for the existing `src/...` import style) | | `outDir` | `"dist"` | **removed** (unused under `tsc --noEmit`) | | `exclude` | _(none)_ | `["dist","node_modules"]` (so the built bundle is never type-checked) | `target`/`lib` for the CLI were already `es2022`. ## Notes / decisions - **Why side-by-side, not a plain `typescript@7` bump:** TS 7.0 is the native (Go) compiler rewrite — feature-identical to 6.0 for type-checking, no programmatic API until 7.1. A plain bump crashed both codegen tools (`Cannot read properties of undefined (reading 'createKeywordTypeNode')`). Side-by-side gives native-TS-7 checking while keeping the TS-6 API for tooling. Once 7.1 ships the API and the tools update, this collapses back to a single `typescript@7` dep. - **`useDefineForClassFields: false` is pinned explicitly.** Raising js-sdk's `target` to `es2022` flips this default to `true`, changing class-field emit and shifting stack frames. The template builder resolves the caller's directory and per-step traces via **fixed-depth** stack walking (`getCallerDirectory` in `src/template/index.ts`), so the extra frames threw it off by one — resolving `.copy('folder/*', …)` against the wrong base dir and mis-attributing build steps (`tests/template/build.test.ts` + `stacktrace.test.ts`). Pinning `false` keeps the exact pre-existing field semantics (es6 already implied `false`); adopting `define` semantics should be a separate, deliberately tested change. - **Target stays at `es2022`, not `es2023`.** `engines` still allow Node 20 (`>=20.18.1 <21 || >=22`). - **`moduleResolution: "bundler"`** typechecks + builds cleanly in both packages. The CLI's `baseUrl`-based bare imports (`from 'src/user'`, `from 'src'`) are preserved via `paths`; the bundled output still resolves them (build verified, binary smoke-tested). ## Not done (intentionally) - **`verbatimModuleSyntax`** — ~177 `import type` conversions; left as a follow-up. - **Shared `tsconfig.base.json`** — the two configs diverge too much to factor out cleanly. ## Verification - `pnpm run typecheck` ✅ both packages, on **native TS 7.0.2** - `pnpm run build` ✅ both packages (js-sdk ESM + CJS + **DTS**; cli CJS; binary smoke-tested) - codegen ✅ `openapi-typescript` + `json2ts` run and produce identical output (idempotent) - `pnpm run lint` ✅ both packages - `pnpm run test` — `template/build` + `template/stacktrace` now pass (`stacktrace` verified locally 30/30); remaining local failures are all `E2B_API_KEY`-gated live tests, unaffected by this change 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ev#1526) Live volume tests create real volumes against the API; this gates them behind an `ENABLE_VOLUME_TESTS` env var so they skip by default. In the JS SDK, the `volumeTest` fixture is chained with `.skipIf(process.env.ENABLE_VOLUME_TESTS === undefined)`, skipping all of `tests/volume/file.test.ts`. In the Python SDK, the `volume` and `async_volume` fixtures call `pytest.skip` when the env var is unset, gating `tests/{sync/volume_sync,async/volume_async}/test_file.py`. Mocked and unit volume tests (msw-based `volume.test.ts`, `test_volume.py`, `test_volume_content.py`, `test_volume_client.py`, `test_volume_connection_config.py`) still run unconditionally. To run the live tests: `ENABLE_VOLUME_TESTS=1 pnpm run test` or `ENABLE_VOLUME_TESTS=1 poetry run pytest`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ntegration (e2b-dev#1524) Replaces the per-call `integration` connection option with a set-once, process-wide setter — `ConnectionConfig.setIntegration()` in JS and `ConnectionConfig.set_integration()` in Python — so integrations wrapping the SDK tag themselves once at startup and every request carries the identifier in the `User-Agent` header, with no threading through individual SDK calls. The setter is internal and hidden from generated docs; the `integration` option is removed from `ConnectionConfigOpts` (kept as a deprecated alias of `ConnectionOpts`) and from the Python constructor, and the round-trip machinery from e2b-dev#1459 is no longer needed since rebuilt configs read the process-wide value. User-Agent handling now follows a single rule in both SDKs via one shared helper per SDK: an explicitly provided `User-Agent` always wins, otherwise the SDK sends its own tagged with the current integration — and SDK-built values are recomputed whenever a config is rebuilt, so clearing or changing the integration propagates. Tests cover attribution, clearing, config rebuilds, and custom User-Agent precedence in both SDKs, with changesets for `e2b` and `@e2b/python-sdk` (minor). CLI attribution using this setter will follow in a separate PR. Usage (internal integrations only): ```ts import { ConnectionConfig } from 'e2b' ConnectionConfig.setIntegration('e2b-code-interpreter/0.1.0') // once at startup ``` ```python from e2b import ConnectionConfig ConnectionConfig.set_integration("e2b-code-interpreter/0.1.0") # once at startup ``` A caller-supplied `User-Agent` (via `headers`/`apiHeaders`) is preserved in both SDKs: ```ts const sbx = await Sandbox.create({ apiHeaders: { 'User-Agent': 'my-app/1.0' } }) // requests carry: my-app/1.0 ``` 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…dev#1549) ## Why `generated_files.yml` only runs on `pull_request`, so its `cache-to: type=gha,mode=max` wrote buildkit blobs into per-PR scopes that other PRs cannot read — every new PR cold-built the codegen image (235–365s in 11 of 17 runs over the past week vs ~65s warm), and ~6 GB of duplicate blobs pushed the repo's Actions cache to 9.9 GB of the 10 GB limit, evicting the Playwright and pnpm caches that e2b-dev#1538 relies on. ## What Adds `codegen_image_cache.yml`, which builds the image on pushes to `main` touching its actual inputs (`codegen.Dockerfile`, `packages/connect-python/**`, or the workflow itself) and exports the cache to main's scope, readable by all PRs; it also supports `workflow_dispatch` for manual re-seeding. The PR-side build in `generated_files.yml` keeps `cache-from` but drops `cache-to`. Merging this PR triggers the first seed automatically, since the new workflow file matches its own paths filter. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
The getHost() JSDoc @example called sandbox.commands.exec(), which is not a method on the Commands class (it exposes run()), so copy-pasting the snippet threw "TypeError: sandbox.commands.exec is not a function". Switch it to sandbox.commands.run(), matching the sibling example in the same file and the Python SDK. Documentation only, no behavior change. Signed-off-by: Anas Khan <83116240+anxkhn@users.noreply.github.com>
Contributor
Author
|
Pushed a revision addressing the latest review/CI feedback. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The JSDoc
@exampleon the publicSandbox.getHost()method callssandbox.commands.exec(...), but theCommandsclass has noexecmethod. Itexposes
run. Copy-pasting the documented snippet therefore throws:Where
packages/js-sdk/src/sandbox/index.ts, in thegetHost()doc comment:The
Commandsclass (packages/js-sdk/src/sandbox/commands/index.ts) exposeslist,sendStdin,closeStdin,kill,connect, andrun(fourrunoverloads), plus a private
start. There is noexec. The correct method hereis
run, which is what every other example already uses, including the sibling@examplein this same file (thecommands.run(...)snippet a few methods up)and both Python SDK mirrors (
get_hostinsandbox_sync/main.pyandsandbox_async/main.pyalready usecommands.run).Fix
One token,
exec->run:Documentation only. No behavior or type change.
Parity with the Python SDK
The repo guidelines ask that SDK changes be mirrored across the JS and Python
SDKs. Here the Python
get_hostexamples already usecommands.runcorrectly,so this defect exists only in the JS SDK doc comment and no Python change is
needed to reach parity.
Tests
This is a JSDoc
@examplecorrection with no runtime code path to exercise, soit adds no test, matching the repo's existing precedent for documentation-only
fixes (e.g.
.changeset/sandbox-list-docstring.md, and merged doc-fix PRs suchas #1511 / #1500 / #1260, none of which added a regression test). Correctness is
that the example now names the real public API: after the change,
commands.execno longer appears anywhere in the SDK source, and
commands.runmatches theCommandsclass and the sibling examples.Offline gates run locally (Node 20, pnpm 9.15.5):
A changeset (
e2b, patch) is included.Linked issues
doc-example defect (the documented snippet throws at runtime). Not filing a
separate issue for a one-token doc fix.
Pre-flight checklist (repo AGENTS.md / CLAUDE.md gates)
pnpm run format-prettier --checkclean on the changed filepnpm run lint- oxlint clean (exit 0)pnpm run typecheck- tsc --noEmit clean (exit 0)pnpm run build- tsc + tsup clean.changeset/fix-gethost-example-command.md(e2b: patch)fix(js-sdk): ..., reusesjs-sdkscope)@example; see Tests section for precedent)@cla-boton the PR after opening (as on prior PRs fix(js-sdk): don't treat "detached" in a branch name as a detached HEAD #1518 / fix(python-sdk): strip colon-separated SGR escape codes in build logs #1519 / fix(python-sdk): percent-encode git credentials in with_credentials #1507)