-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ENH] Bundle CLI in JS client #4200
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Reviewer ChecklistPlease leverage this checklist to ensure your code review is thorough before approving Testing, Bugs, Errors, Logs, Documentation
System Compatibility
Quality
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how do we plan on versioning the bindings packages?
@codetheweb I think for versioning, we should follow the same schema for the python bindings if any. The plan is to create the full set of bindings for JS too eventually |
Ok, should the version for each platform package (e.x. |
Never mind, sorry. Just saw your follow-up task in OP. |
It's ok, we'll work it out on the remix |
uses: actions-rs/toolchain@v1 | ||
with: | ||
toolchain: stable | ||
override: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did using our rust setup action not work here? we should ideally use it not just for dry but also because it reads from rust-toolchain.toml
as the source of truth for the toolchain version
it also uses sccache so you don't need to handle caching (not entirely convinced the cache action setup here is correct)
## Description of changes The `rust/js_bindings` crate is a `napi` project exposing the CLI entry point in [`src/lib.rs`](https://github.com/chroma-core/chroma/blob/main/rust/js_bindings/src/lib.rs). Inside this crate there is also an `npm` directory defining platform specific packages for these JS bindings. Note that every package specifies the platform it supports. These packages are published by the `build-js-bindings.yml` workflow. Our main JS client, `clients/js/packages/chromadb`, specifies all platforms as optional dependencies. When `npm install chromadb`ing, the correct optional dependency will be installed. There, `bindings.ts` exposes that package to `cli.ts`, where we wrap the entry point. Follow up work: - Script for better local dev experience. - Update JS release runbook for updating bindings versions. ## Test plan *How are these changes tested?* - [ ] Tests pass locally with `pytest` for python, `yarn test` for js, `cargo test` for rust ## Documentation Changes *Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the [docs repository](https://github.com/chroma-core/docs)?*
Description of changes
The
rust/js_bindings
crate is anapi
project exposing the CLI entry point insrc/lib.rs
.Inside this crate there is also an
npm
directory defining platform specific packages for these JS bindings. Note that every package specifies the platform it supports. These packages are published by thebuild-js-bindings.yml
workflow.Our main JS client,
clients/js/packages/chromadb
, specifies all platforms as optional dependencies. Whennpm install chromadb
ing, the correct optional dependency will be installed. There,bindings.ts
exposes that package tocli.ts
, where we wrap the entry point.Follow up work:
Test plan
How are these changes tested?
pytest
for python,yarn test
for js,cargo test
for rustDocumentation Changes
Are all docstrings for user-facing APIs updated if required? Do we need to make documentation changes in the docs repository?