feat(sdk): public withOptions / with_params on resource classes - #1786
feat(sdk): public withOptions / with_params on resource classes#1786devin-ai-integration[bot] wants to merge 8 commits into
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…sses Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
🦋 Changeset detectedLatest commit: 80385e7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Package ArtifactsBuilt from c20f0b8. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.46.2-devin-1787866410-with-opts-multi-client.0.tgzCLI ( npm install ./e2b-cli-2.18.1-devin-1787866410-with-opts-multi-client.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.7.3-devin-1787866410-with-opts-multi-client.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.3.4-devin-1787866410-with-opts-multi-client.0.tgzPython SDK ( pip install ./e2b-2.46.1+devin.1787866410.with.opts.multi.client-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.9.2+devin.1787866410.with.opts.multi.client-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.4.5+devin.1787866410.with.opts.multi.client-py3-none-any.whl |
There was a problem hiding this comment.
Reviewed against TASTE.md (sdk-harness), focusing on the JS/Python parity rules (T-1, T-1a, T-2), API-shape rules (T-4, T-18, T-19, T-22, T-23), and the configuration rules (T-49, T-51). The refactor itself is a good move — the binding logic now lives in one place per language and the merge precedence is unchanged.
2 violations, both on the JS side:
- T-1 / T-18 —
withOptsis a public static on every resource class (Sandbox.withOpts(...)type-checks and autocompletes for users;@hiddenonly hides it from generated docs), while its Python mirror_with_paramsis private. See inline comment. - T-23 — the option type is spelled inline as
Omit<ConnectionOpts, 'signal'>even thoughE2BClientOptsalready names exactly that shape and is exported. See inline comment.
Two notes not tied to a changed line:
- T-51 (config objects are immutable snapshots): both new doc comments claim that copying the options means "later mutations of the caller's object/dicts cannot change the bound configuration". That holds only at the top level —
headers,apiHeadersandproxyare still the caller's objects by reference, so mutatingopts.headersafter construction does change the bound config. Either say "shallow copy" or copy the nested maps. - The
class extends (this as unknown as typeof ClientFactory) { … } as unknown as Tdouble cast means nothing in this function is actually checked by TS — theT extends { prototype: ClientFactory }constraint accepts any object with aprototype, sowithOptscalled on an unrelated class fails only at runtime. Thethis-typed static plus cast is the pragmatic option given TS has no polymorphicthisfor statics (and the comment explains that well), but if the method stops being part of the public surface (point 1) the constraint can be tightened totypeof ClientFactory & { prototype: ClientFactory }and only the return cast kept.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a41b0e43b5
ℹ️ 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".
There was a problem hiding this comment.
Looks good — a clean, mechanical refactor that moves the existing bind-and-strip-signal logic from E2B's constructor into a shared ClientFactory.withOpts/_with_params method, with no behavior change. Reviewed the JS withOpts (signal-stripping copy, this-typed generic tightened in the second commit) and Python _with_params (kwargs-unpacking dict copy via type(...)) for parity, and confirmed no stale references to the old _bind helper or inline boundOpts subclassing remain. Existing tests (boundOpts.test.ts, test_bound_api_params.py, test_client.py) exercise the new code path, and the patch changeset is appropriately scoped since the new methods are marked internal/hidden.
Extended reasoning...
Overview
This PR refactors how E2B's multi-client binds connection options to its resource classes (Sandbox, Volume, Secret, Template, and the Python async variants). The bind-a-subclass-with-config logic that previously lived inline in E2B's constructor (JS anonymous class extends X { boundOpts }, Python _bind() helper) is moved onto the shared ClientFactory base as withOpts (JS) / _with_params (Python), and client.ts/client.py are simplified to call it per resource class.
Security risks
None identified. This is purely an internal code-organization change; no new external input handling, auth, or crypto is touched. The signal-stripping behavior (to avoid an AbortSignal bleeding into every call) is preserved verbatim in JS, and the Python side never had a signal concept in ApiParams to begin with, so parity holds.
Level of scrutiny
Low-to-moderate. The change is small, self-contained, and mechanical (moving code, not changing semantics), but it touches a shared base class (ClientFactory) used by both sync and async resource classes in Python and all resource classes in JS, so I checked parity carefully: JS's withOpts still copies opts and deletes signal; Python's _with_params relies on **params kwargs-unpacking to produce a fresh dict per call (previously a single shared dict was reused across all 8 _bind calls in client.py, which is a harmless behavioral non-issue since the dict was never mutated). I verified no stale _bind( or old inline boundOpts = subclass patterns remain in the codebase after the refactor.
Other factors
Existing tests (packages/js-sdk/tests/template/boundOpts.test.ts, packages/python-sdk/tests/sync/template_sync/test_bound_api_params.py, packages/python-sdk/tests/async/template_async/test_bound_api_params.py, and the integration-style packages/python-sdk/tests/test_client.py) exercise the exact code paths touched here and were not modified, consistent with the "behavior unchanged" claim. A patch-level changeset was added for both e2b and @ e2b/python-sdk, which is appropriate since the new methods are marked @ internal @ hidden / :meta private: rather than being new public API. The second commit tightens the JS generic constraint from <T> to <T extends { prototype: ClientFactory }>, a real type-safety improvement caught during the PR's own iteration. I was unable to execute pnpm run typecheck/tests directly due to a sandbox permission restriction, so this assessment relies on careful static reading of the diff plus the pre-existing test coverage.
|
okay let's instead switch to an exported method bindClientOpts / bind_client_params |
…with already-bound opts Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…clarify shallow-copy docs Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…t_params helpers instead of statics Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
…w merged client; make binding helpers internal Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
|
I am thinking of keeping it as-is for now and redoing how we do the client inside the classes |
Summary
Reworks the multi-client mechanism: the resource classes are themselves clients, so they gain a public
withOptions(JS static) /with_params(Python classmethod) that returns a copy of the class with the connection config bound, merging with any config already bound. TheE2Bclient is built on top of these.withOptions/with_paramsreturn a new bound subclass; the receiver is unchanged. Rebinding merges:{...alreadyBound, ...new}, and per-call options still take precedence over bound options, with env vars as the fallback.Template.withOptionsreturns a callable template, soMyTemplate()keeps working as a builder factory.signalso it is never bound; options are shallow-copied so caller mutations can't affect a bound class.bindClientOpts/bind_client_paramssubclassing helpers stay internal (not exported from the package entrypoints);withOptions/with_paramsare the public API, inherited by anyClientFactorysubclass so downstream SDKs (desktop, code-interpreter) get it on their resources for free.new E2B(opts)now simply callsResource.withOptions(opts)for each resource; the client itself has nowithOptions/with_params.Link to Devin session: https://app.devin.ai/sessions/56eb7b178bad4472a975e6607475af43
Open in Devin Desktop: https://app.devin.ai/desktop/session/56eb7b178bad4472a975e6607475af43?variant=devin
Requested by: @mishushakov