refactor(node): move rest bag adaptation into the napi binding#536
Merged
Conversation
napi-rs registers class statics as non-writable AND non-configurable. `lib/index.ts` reassigned `JsBoxlite.rest` to adapt the native positional `rest(url, credential?, prefix?)` to the public `BoxliteRestOptions` bag, which threw `TypeError: Cannot assign to read only property 'rest'` at module import — breaking every consumer of the SDK (3 integration suites failed on import). A Proxy over the native class is also rejected: a `get` trap may not substitute a non-configurable, non-writable own data property. So the Proxy target is a fresh function that owns no such property; `rest` resolves to the bag adapter while construction and every other static forward to the native class untouched. Adds rest-binding.integration.test.ts covering import + bag adaptation.
The native `rest` was the lone positional binding method while every other option is a struct; `lib/index.ts` bridged the public `BoxliteRestOptions` bag to it by wrapping the native class in a JS Proxy (PR #535 hotfix). napi-rs idiom — and our own Python SDK (`PyBoxliteRestOptions` + `impl From`) — is to shape option bags in Rust, not reshape native objects in JS. Add a `#[napi]` `JsBoxliteRestOptions` class with `into_core()` (the napi twin of Python's `From`), retarget `JsBoxlite::rest` to take it, and replace the Proxy with a plain ES subclass that restates the bag-taking `rest` and inherits `new`/`withDefaultConfig`/`initDefault`. The public TypeScript API is byte-identical; `lib/options.ts` stays pure-TS (preserves the object-bag ctor + credential identity). Verified napi-rs class subclassing works here (napi-rs #1164 risk did not materialize): added integration coverage for inherited construct + static paths, plus a Rust unit test for `into_core`.
5 tasks
CI flagged clippy::wrong_self_convention on `into_core(&self)` (an `into_*` method must take self by value) plus a rustfmt diff. Replace the inherent method with `impl From<&JsBoxliteRestOptions> for BoxliteRestOptions` — borrowed source since napi passes class args by reference — which also tracks Python's `impl From` more closely. Apply rustfmt.
2 tasks
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
1 participant
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.
Summary
Follow-up to #535 (the Proxy hotfix). Research (napi-rs docs + @node-rs/*, swc, Prisma, lightningcss, and our own Python SDK) confirmed the Proxy is a non-idiomatic workaround: option-bag adaptation belongs in Rust. The Python SDK already does this for the identical concept (
sdks/python/src/options.rs:858PyBoxliteRestOptions+impl From, consumed byruntime.rs:50).#[napi] JsBoxliteRestOptions+into_core()insdks/node/src/options.rs— the napi twin of Python'sFrom.JsBoxlite::restto take&JsBoxliteRestOptions(was the lone positional binding method).lib/index.ts; replace with a plain ES subclass that restates the bag-takingrestand inheritsnew/withDefaultConfig/initDefault.lib/options.tsstays pure-TS → public TypeScript API byte-identical;new BoxliteRestOptions({...})andcredentialidentity preserved.Stacked on
fix/node-rest-binding-readonly(#535); supersedes its Proxy. Merge #535 first (or rebase onto main after).napi-rs #1164 (class subclassing) — verified, not a risk here
Added integration coverage proving inherited construction (
new JsBoxlite({homeDir})viasuper()) and the inheritedwithDefaultConfigstatic both work. No fallback needed.Test plan
make dev:node— Rust compiles, napi bindings regenerated, tsc cleanrest_options_into_core_maps_fields(cargo test -p boxlite-node) passesmake test:integration:node: 9 passed / 1 skipped, 46 passed / 17 skipped, exit 0make testgreen on push