-
Notifications
You must be signed in to change notification settings - Fork 23
[PM-33733] Reorganize SDK documentation #793
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
Changes from all commits
215a149
1dbb1a9
094eb5b
569256d
6d52f2c
d326d73
2d198c3
4d41e12
b9bf623
a0245dd
c369611
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,6 +57,7 @@ OTLP | |
| owasp | ||
| passcode | ||
| passwordless | ||
| passthroughs | ||
| pinentry | ||
| PNSs | ||
| POSIX | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,213 @@ | ||
| --- | ||
| sidebar_position: 1 | ||
| --- | ||
|
|
||
| # Adding new functionality | ||
|
|
||
| This guide walks through the steps for adding new functionality to the SDK. For guidance on whether | ||
| something belongs in the SDK at all, see | ||
| [What belongs in the SDK](index.md#what-belongs-in-the-sdk). | ||
|
|
||
| The examples below use `FoldersClient` from `bitwarden-vault` as a reference. For guidance on how to | ||
| organize the files within a client, see [Client patterns](client-patterns.md). | ||
|
|
||
| ## 1. Readiness checklist | ||
|
|
||
| Before creating a new crate or adding to an existing one, work through these questions to identify | ||
| any blockers or prerequisites. | ||
|
|
||
| - **Does the functionality depend on other code that is not yet in the SDK?** Moving it is not | ||
| recommended until those dependencies are available. Consider asking the team that owns the | ||
| upstream code to migrate it first. | ||
|
|
||
| - **Does the functionality require authenticated API requests?** The SDK supports authenticated | ||
| requests through autogenerated bindings. See [`bitwarden-vault`][vault-crate] as an example of a | ||
| crate that makes authenticated calls. | ||
|
|
||
| - **Does the functionality require persistent state?** Review the docs for | ||
| [`bitwarden-state`][state-crate] and see [`bitwarden-vault`][vault-crate] for an example of how | ||
| state is managed. | ||
|
|
||
| - **Does the functionality need the SDK to produce an observable or reactive value?** Migrate the | ||
| business logic to the SDK and build reactivity on top of it in TypeScript. | ||
|
|
||
| ## 2. Create the crate | ||
|
|
||
| When the functionality warrants its own crate β typically when it represents a distinct domain β add | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add more guidance on whether to create a crate vs reuse an existing one, or how big the scope of a crate should be?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is, but I feel it's partially up to the teams to identify that. |
||
| a new crate under the `crates/` directory in the | ||
| [SDK repository](https://github.com/bitwarden/sdk-internal). | ||
|
|
||
| 1. Create the crate with `cargo init` and add it to the workspace `Cargo.toml`. | ||
| 2. Add `bitwarden-core` as a dependency for the shared runtime. | ||
| 3. Configure `CODEOWNERS` to ensure the appropriate team is assigned to review changes to the crate. | ||
|
|
||
| ## 3. Define the client struct | ||
|
|
||
| Each feature crate exposes one or more client structs that group related operations. Create a struct | ||
| that holds the dependencies the client needs and use the `#[derive(FromClient)]` macro to | ||
| automatically populate fields from the SDK `Client`. See | ||
| [Client patterns β `FromClient` and dependency injection](client-patterns.md#fromclient-and-dependency-injection) | ||
| for details on how this works and which dependency types are available. | ||
|
|
||
| ```rust | ||
| #[derive(FromClient)] | ||
| pub struct FoldersClient { | ||
| pub(crate) key_store: KeyStore<KeyIds>, | ||
| pub(crate) api_configurations: Arc<ApiConfigurations>, | ||
| pub(crate) repository: Option<Arc<dyn Repository<Folder>>>, | ||
| } | ||
| ``` | ||
|
|
||
| If the client will be exposed over WASM, annotate it with | ||
| `#[cfg_attr(feature = "wasm", wasm_bindgen)]`. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't mention when creating the crate above that the feature needs to exist. Maybe we just always instruct to add a uniffi/wasm feature to all feature crates?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This ties into my thought for having a template. |
||
|
|
||
| ## 4. Wire into the application interface | ||
|
|
||
| Connect the feature client to the SDK `Client` by defining an | ||
| [extension trait](client-patterns.md#extension-traits). This makes the feature accessible without | ||
| modifying `Client` itself. | ||
|
dani-garcia marked this conversation as resolved.
|
||
|
|
||
| ```rust | ||
| pub trait VaultClientExt { | ||
| fn vault(&self) -> VaultClient; | ||
| } | ||
|
|
||
| impl VaultClientExt for Client { | ||
| fn vault(&self) -> VaultClient { | ||
| VaultClient::new(self.clone()) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| :::note | ||
|
|
||
| While there is a team called `vault`, `VaultClient` refers to the vault-domain, not the team. You | ||
| should not create team-clients in the SDK. Instead, organize clients by domain or feature area, and | ||
| assign ownership to the team that maintains that domain or feature. | ||
|
|
||
| ::: | ||
|
|
||
| For larger domains, the application interface client delegates to sub-clients rather than | ||
| implementing every method itself. For example, `VaultClient` exposes `FoldersClient`, | ||
| `CiphersClient`, and others through accessor methods: | ||
|
|
||
| ```rust | ||
| #[cfg_attr(feature = "wasm", wasm_bindgen)] | ||
| impl VaultClient { | ||
| pub fn folders(&self) -> FoldersClient { | ||
| FoldersClient::from_client(&self.client) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Finally, expose the new client in the application interface entry point so consumers can reach it. | ||
| For the Password Manager SDK, this means adding an accessor method to | ||
| [`PasswordManagerClient`][pm-lib]: | ||
|
|
||
| ```rust | ||
| impl PasswordManagerClient { | ||
| pub fn vault(&self) -> bitwarden_vault::VaultClient { | ||
| self.0.vault() | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| :::tip | ||
|
|
||
| Steps 2β4 (create crate, define client, wire into application interface) should be submitted as a | ||
| single pull request. Keep it small and focused on scaffolding β the Platform team reviews additions | ||
| to the application interface clients, so a narrow scope helps move that review along quickly. | ||
|
|
||
| ::: | ||
|
|
||
| ## 5. Implement methods | ||
|
|
||
| With the crate scaffolding merged, add methods in subsequent pull requests. Each method should own | ||
| its logic directly on the client struct β avoid thin passthroughs to free functions. For larger | ||
| clients, split methods into separate files or subdirectories as described in | ||
| [Client patterns](client-patterns.md#per-method-files-or-subdirectories). | ||
|
|
||
| :::important | ||
|
|
||
| Every public method on a client is a contract with consumers and must have test coverage. Treat | ||
| client methods as a public API boundary β changes to their behavior can break downstream consumers | ||
| across multiple platforms. See [Client patterns β Testing](client-patterns.md#testing) for how to | ||
| set up test doubles. | ||
|
|
||
| ::: | ||
|
|
||
| ```rust | ||
| #[cfg_attr(feature = "wasm", wasm_bindgen)] | ||
| impl FoldersClient { | ||
| pub async fn get(&self, folder_id: FolderId) -> Result<FolderView, GetFolderError> { | ||
| let folder = self | ||
| .repository | ||
| .require()? | ||
| .get(folder_id) | ||
| .await? | ||
| .ok_or(ItemNotFoundError)?; | ||
|
|
||
| Ok(self.key_store.decrypt(&folder)?) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| Consumers access the feature through the application interface: | ||
|
|
||
| ```rust | ||
| let folders = client.vault().folders().list().await?; | ||
| ``` | ||
|
|
||
|
dani-garcia marked this conversation as resolved.
|
||
| ## 6. Add mobile bindings | ||
|
|
||
| If the new functionality needs to be available on mobile platforms (Android / iOS), add a | ||
| [UniFFI](language-bindings.md#mobile-bindings) wrapper in the `bitwarden-uniffi` crate. | ||
|
|
||
| ### Expose the client | ||
|
|
||
| Add an accessor method on the appropriate UniFFI client β typically in | ||
| [`bitwarden-uniffi/src/lib.rs`][uniffi-lib] or a sub-client β that returns a new wrapper struct: | ||
|
|
||
| ```rust | ||
| impl Client { | ||
| pub fn vault(&self) -> Arc<VaultClient> { | ||
| Arc::new(VaultClient(self.0.clone())) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| ### Create the wrapper | ||
|
|
||
| Create a wrapper struct that holds the SDK `Client` and delegates to the underlying Rust client. See | ||
| [`bitwarden-uniffi/src/tool/sends.rs`][uniffi-sends] for a complete example. | ||
|
|
||
| ```rust | ||
| use crate::Result; | ||
|
|
||
| pub struct FoldersClient(pub(crate) SharedClient); | ||
|
|
||
| #[uniffi::export] | ||
| impl FoldersClient { | ||
| pub async fn get(&self, folder_id: FolderId) -> Result<FolderView> { | ||
| Ok(self.0.vault().folders().get(folder_id).await?) | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| The wrapper should convert errors into `BitwardenError`. When introducing a new error type, add a | ||
| variant for it in [`bitwarden-uniffi/src/error.rs`][uniffi-error] and implement the `From` | ||
| conversion. | ||
|
|
||
| ## Ownership | ||
|
|
||
| Feature and domain crates are usually owned and maintained by individual teams. When creating a new | ||
| crate, coordinate with the Platform team to establish ownership and review expectations. | ||
|
|
||
| [pm-lib]: https://github.com/bitwarden/sdk-internal/blob/main/crates/bitwarden-pm/src/lib.rs | ||
| [state-crate]: https://github.com/bitwarden/sdk-internal/tree/main/crates/bitwarden-state | ||
| [uniffi-error]: | ||
| https://github.com/bitwarden/sdk-internal/blob/main/crates/bitwarden-uniffi/src/error.rs | ||
| [uniffi-lib]: https://github.com/bitwarden/sdk-internal/blob/main/crates/bitwarden-uniffi/src/lib.rs | ||
| [uniffi-sends]: | ||
| https://github.com/bitwarden/sdk-internal/blob/main/crates/bitwarden-uniffi/src/tool/sends.rs | ||
| [vault-crate]: https://github.com/bitwarden/sdk-internal/tree/main/crates/bitwarden-vault | ||
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.
I wonder if we could make a template using https://crates.io/crates/cargo-generate to streamline this process. Have two templates, one for feature crates and another for util crates.
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.
I've had good luck letting claude do it in the past, but I agree a utility that would create the crate structure, manifest and properly configured client would be nice
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.
A new Claude skill to add to the
ai-pluginsmarketplace perhaps π€