From fdeb40a2652adbdfcc0d603f755395bf5c0a380d Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Fri, 10 Nov 2023 08:07:09 -0500 Subject: [PATCH 01/20] Add State Provider Framework Deep Dive --- docs/architecture/deep-dives/state/index.md | 73 +++++++++++++++++++++ 1 file changed, 73 insertions(+) create mode 100644 docs/architecture/deep-dives/state/index.md diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md new file mode 100644 index 00000000..25232937 --- /dev/null +++ b/docs/architecture/deep-dives/state/index.md @@ -0,0 +1,73 @@ +# State Provider Framework + +The state provider framework was designed for the purpose of allowing state to be owned by domains but also to enforce good practices, reduce boilerplate around account switching, and provide a trustworthy observable stream of that state. + +An example usage of the framework is below: + +```typescript +import { FOLDERS_USER_STATE, FOLDERS_GLOBAL_STATE } from "../key-definitions"; + +class FolderService { + private folderGlobalState: GlobalState; + private folderUserState: UserState; + + constructor( + private userStateProvider: UserStateProvider, + private globalStateProvider: GlobalStateProvider + ) { + this.folderUserState = userStateProvider.get(FOLDERS_USER_STATE); + this.folderGlobalState = globalStateProvider.get(FOLDERS_GLOBAL_STATE); + } + + get folders$(): Observable { + return this.folderUserState.pipe(map(folders => this.transform(folders))); + } +} +``` + +The constructor takes in 2 new interfaces that both expose a `get` method that takes a single argument, a `KeyDefinition` that should be imported from a file where it was defined as a `const`. A `KeyDefinition` has the following structure. + +```typescript +class KeyDefinition { + stateDefinition: StateDefinition; + key: string; + deserializer: (jsonValue: Jsonify) => T; +} +``` + +If a service is stateless or only needs one of either global or user state they only need to take the specific provider that they need. But if you do need state and you call the `get` methods you are returned `UserState` and `GlobalState` respectively. They both expose 2 common properties, those are `state$: Observable` and `update(configureState: (state: T) => T): Promise`. + + +The `state$` property is a stream of state in the shape of the generic `T` that is defined on the `KeyDefinition` passed +into the providers `get` method. The `state$` observable only subscribes to it's upstream data sources when it itself is subscribed to. + +The `update` property is a method that you call with your own function that updates the current state with your desired +shape. The callback style of updating state is such that you can make additive changes to your state vs just replacing the +whole value (which you can still do). An example of this could be pushing an item onto an array. + +The `update` method on both `UserState` and `GlobalState` will cause an emission to the associated `state$` observable. For `GlobalState` this is the only way the `state$` observable will update, but for `UserState` it will emit when the currently active user updates. When that updates the observable will automatically update with the equivalent +data for that user. + +## UserState<T> + +UserState includes a few more API's that help you interact with a users state, it has a `updateFor` and `createDerived`. + +The `updateFor` method takes a `UserId` in its first argument that allows you to update the state for a specified user +instead of the user that is currently active. Its second argument is the same callback that exists in the `update` method. + +The `createDerived` method looks like this: + +```typescript +createDerived(derivedStateDefinition: DerivedStateDefinition): DerivedUserState +``` + +and the definition of `DerivedStateDefinition` looks like: + +```typescript +class DerivedStateDefinition { + keyDefinition: KeyDefinition; + converter: (data: TFrom, context: { activeUserKey: UserKey, encryptService: EncryptService}) => Promise +} +``` + +This class encapsulates the logic for how to go from one version of your state into another form. This is often because encrypted data is stored on disk, but decrypted data is what will be shown to the user. From ed814a505080c83584c914295397fb2514396aeb Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Tue, 5 Dec 2023 15:13:47 -0500 Subject: [PATCH 02/20] Update Deep Dive --- docs/architecture/deep-dives/state/index.md | 307 +++++++++++++++++--- 1 file changed, 271 insertions(+), 36 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 25232937..7a54785f 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -1,73 +1,308 @@ # State Provider Framework -The state provider framework was designed for the purpose of allowing state to be owned by domains but also to enforce good practices, reduce boilerplate around account switching, and provide a trustworthy observable stream of that state. +The state provider framework was designed for the purpose of allowing state to be owned by domains +but also to enforce good practices, reduce boilerplate around account switching, and provide a +trustworthy observable stream of that state. -An example usage of the framework is below: +Core API's: + +## API's + +- [`StateDefinition`](#statedefinition) +- [`KeyDefinition`](#keydefinition) +- [`StateProvider`](#stateprovider) +- [`ActiveUserState`](#activeuserstatet) +- [`GlobalState`](#globalstatet) +- [`SingleUserState`](#singleuserstatet) + +### `StateDefinition` + +`StateDefinition` is a simple API but a very core part of making the State Provider Framework work +smoothly. Teams will interact with it only in a single, `state-definitions.ts`, file in the +[`clients`](https://github.com/bitwarden/clients) repository. This file is located under platform +code ownership but teams are expected to create edits to it. A team will edit this file to include a +line like such: + +```typescript +export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk"); +``` + +The first argument to the `StateDefinition` constructor is expected to be a human readable, +camelCase formatted name for your domain, or state area. The second argument will either be the +string literal `"disk"` or `"memory"` dictating where all the state using this `StateDefinition` +should be stored. The platform team will be responsible to reviewing all new and updated entries in +this file and will be looking to make sure that there are no duplicate entries containing the same +state name and state location. Teams CAN have the same state name used for both `"disk"` and +`"memory"` locations. Tests are included to ensure this uniqueness and core naming guidelines so you +can ensure a review for a new `StateDefinition` entry can be done promptly and with very few +surprises. + +_TODO: Make tests_ + +:::note + +Secure storage is not currently supported as a storage location in the State Provider Framework. If +you need to store data in the secure storage implementations, please continue to use `StateService`. +The Platform team will weigh the possibility of supporting secure storage down the road. + +::: + +### `KeyDefinition` + +`KeyDefinition` builds on the idea of [`StateDefinition`](#statedefinition) but it gets more +specific about the data to be stored. `KeyDefinition`s can also be instantiated in your own teams +code. This might mean creating it in the same file as the service you plan to consume it or you may +want to have a single `key-definitions.ts` file that contains all the entries for your team. Some +example instantiations are: + +```typescript +const MY_DOMAIN_DATA = new KeyDefinition(MY_DOMAIN_DISK, "data", { + deserializer: (jsonData) => null, // convert to your data from json +}); + +// Or if your state is an array, use the built in helper +const MY_DOMAIN_DATA: KeyDefinition = KeyDefinition.array( + MY_DOMAIN_DISK, + "data", + { + deserializer: (jsonDataElement) => null, // provide a deserializer just for the element of the array + }, +); + +// record +const MY_DOMAIN_DATA: KeyDefinition> = + KeyDefinition.record(MY_DOMAIN_DISK, "data", { + deserializer: (jsonDataValue) => null, // provide a deserializer just for the value in each key-value pair + }); +``` + +The first argument to `KeyDefinition` is always the `StateDefinition` that this key should belong +to. The second argument should be a human readable, camelCase formatted, name of the +`KeyDefinition`. This name should be unique amongst all other `KeyDefinition`s that consume the same +`StateDefinition`. The responsibility of this uniqueness is on the team. As such, you should only +consume the `StateDefinition` of another team in your own `KeyDefinition` very rarely and if it is +used, you should practice extreme caution, explicit comments stating such, and with coordination +with the team that owns the `StateDefinition`. + +### `StateProvider` + +`StateProvider` is an injectable service that includes 3 methods for getting state. These three +methods are helpers for invoking their more modular siblings `ActiveStateProvider.get`, +`SingleUserStateProvider.get`, and `GlobalStateProvider.get` and they have the following type +definitions: + +```typescript +interface StateProvider { + getActive(keyDefinition: KeyDefinition): ActiveUserState; + getUser(userId: UserId, keyDefinition: KeyDefinition): SingleUserState; + getGlobal(keyDefinition: KeyDefinition): GlobalState; +} +``` + +A very common practice will be to inject `StateProvider` in your services constructor and call +`getActive`, `getGlobal`, or both in your constructor and then store private properties for the +resulting `ActiveUserState` and/or `GlobalState`. It's less common to need to call `getUser` +in the constructor because it will require you to know the `UserId` of the user you are attempting +to edit. Instead you will add `private` to the constructor argument injecting `StateProvider` and +instead use it in a method like in the below example. ```typescript import { FOLDERS_USER_STATE, FOLDERS_GLOBAL_STATE } from "../key-definitions"; class FolderService { private folderGlobalState: GlobalState; - private folderUserState: UserState; - - constructor( - private userStateProvider: UserStateProvider, - private globalStateProvider: GlobalStateProvider - ) { - this.folderUserState = userStateProvider.get(FOLDERS_USER_STATE); - this.folderGlobalState = globalStateProvider.get(FOLDERS_GLOBAL_STATE); + private folderUserState: ActiveUserState>; + + folders$: Observable; + + constructor(private stateProvider: StateProvider) { + this.folderUserState = stateProvider.getActive(FOLDERS_USER_STATE); + this.folderGlobalState = stateProvider.getGlobal(FOLDERS_GLOBAL_STATE); + + this.folders$ = this.folderUserState.pipe( + map((foldersRecord) => this.transform(foldersRecord)), + ); } - get folders$(): Observable { - return this.folderUserState.pipe(map(folders => this.transform(folders))); + async clear(userId: UserId): Promise { + await this.stateProvider.getUser(userId, FOLDERS_USER_STATE).update((state) => null); } } ``` -The constructor takes in 2 new interfaces that both expose a `get` method that takes a single argument, a `KeyDefinition` that should be imported from a file where it was defined as a `const`. A `KeyDefinition` has the following structure. +### ActiveUserState\ + +`ActiveUserState` is an object to help you maintain and view the state of the currently active +user. If the currently active user changes, like through account switching. The data this object +represents will change along with it. Gone is the need to subscribe to +`StateService.activeAccountUnlocked$`. You can see the most likely type definition of the API's on +`ActiveUserState` below: ```typescript -class KeyDefinition { - stateDefinition: StateDefinition; - key: string; - deserializer: (jsonValue: Jsonify) => T; +interface ActiveUserState { + state$: Observable; + update(updateState: (state: T) => T): Promise; } ``` -If a service is stateless or only needs one of either global or user state they only need to take the specific provider that they need. But if you do need state and you call the `get` methods you are returned `UserState` and `GlobalState` respectively. They both expose 2 common properties, those are `state$: Observable` and `update(configureState: (state: T) => T): Promise`. +:::note + +The definition of `update` shown above is not complete and the full definition includes more +customization that you can view in the [Advanced Usage](#advanced-usage) section. + +::: +The `update` method takes a function `updateState: (state: T) => T` that can be used to update the +state in both a destructive and additive way. The function gives you a representation of what is +currently saved as your state and it requires you to return the state that you want saved into +storage. This means if you have an array on your state, you can `push` onto the array and return the +array back. -The `state$` property is a stream of state in the shape of the generic `T` that is defined on the `KeyDefinition` passed -into the providers `get` method. The `state$` observable only subscribes to it's upstream data sources when it itself is subscribed to. +The `state$` property provides you an `Observable` that can be subscribed to. +`ActiveUserState.state$` will emit for the following reasons: -The `update` property is a method that you call with your own function that updates the current state with your desired -shape. The callback style of updating state is such that you can make additive changes to your state vs just replacing the -whole value (which you can still do). An example of this could be pushing an item onto an array. +- The active user changes. +- You call the `update` method. +- Another service in a different context calls `update` on their own instance of + `ActiveUserState` made from the same `KeyDefinition`. +- A `SingleUserState` method pointing at the same `KeyDefinition` as `ActiveUserState` and + pointing at the user that is active that had `update` called -The `update` method on both `UserState` and `GlobalState` will cause an emission to the associated `state$` observable. For `GlobalState` this is the only way the `state$` observable will update, but for `UserState` it will emit when the currently active user updates. When that updates the observable will automatically update with the equivalent -data for that user. +### `GlobalState` -## UserState<T> +`GlobalState` has an incredibly similar API surface as `ActiveUserState` except it targets +global scoped storage and does not emit an update to `state$` when the active user changes, only +when the `update` method is called, in this context, or another. -UserState includes a few more API's that help you interact with a users state, it has a `updateFor` and `createDerived`. +### `SingleUserState` -The `updateFor` method takes a `UserId` in its first argument that allows you to update the state for a specified user -instead of the user that is currently active. Its second argument is the same callback that exists in the `update` method. +`SingleUserState` behaves very similarly to `GlobalState` where neither will react to active +user changes and you instead give it the user you want it to care about up front. Because of that +`SingleUserState` exposes a `userId` property so you can see the user this instance is about. It +also has an interesting way it can interact with `ActiveUserState`, if you have pointed +`SingleUserState` at user who happens to be active at the time calling `update` from either of +those services will cause an emission from both of their `state$` observables. -The `createDerived` method looks like this: +## Migrating + +Migrating data to state providers is incredibly similar to migrating data in general. You create +your own class that extends `Migrator`. That will require you to implement your own +`migrate(migrationHelper: MigrationHelper)` method. `MigrationHelper` already includes methods like +`get` and `set` for getting and settings value to storage by their string key. There are also +methods for getting and setting using your `KeyDefinition` or `KeyDefinitionLike` object to and from +user and global state. An example of how you might use these new helpers is below: + +```typescript +type ExpectedGlobalState = { myGlobalData: string }; + +type ExpectedAccountState = { myUserData: string }; + +const MY_GLOBAL_KEY_DEFINITION: KeyDefinitionLike = { + stateDefinition: { name: "myState" }, + key: "myGlobalKey", +}; +const MY_USER_KEY_DEFINITION: KeyDefinitionLike = { + stateDefinition: { name: "myState" }, + key: "myUserKey", +}; + +export class MoveToStateProvider extends Migrator<10, 11> { + async migrate(migrationHelper: MigrationHelper): Promise { + const existingGlobalData = await migrationHelper.get("global"); + + await migrationHelper.setGlobal(MY_GLOBAL_KEY_DEFINITION, { + myGlobalData: existingGlobalData.myGlobalData, + }); + + const updateAccount = async (userId: string, account: ExpectedAccountState) => { + await migrationHelper.setUser(MY_USER_KEY_DEFINITION, { + myUserData: account.myUserData, + }); + }; + + const accounts = await migrationHelper.getAccounts(); + + await Promise.all(accounts.map(({ userId, account }) => updateAccount(userId, account))); + } +} +``` + +:::note + +`getAccounts` only gets data from the legacy account object that was used in `StateService`. As data +gets migrated off of that account object the response from `getAccounts`, which returns a record +where the key will be a users id and the value being the legacy account object. It can continue to +be used to retrieve the known authenticated user ids though. + +::: + +### Example PRs + +_TODO: Include PR's_ + +## Testing + +Testing business logic with data and observables can sometimes be cumbersome, to help make that a +little easier, there are a sweet of helpful "fakes" that can be used instead of traditional "mocks". +Now instead of calling `mock()` into your service you can instead use +`new FakeStateProvider()`. + +_TODO: Refine user story_ + +## Advanced Usage + +### `update` + +It was mentioned earlier that update has the type signature +`update(updateState: (state: T) => T): Promise` but that is not entirely true. You can treat it +as if it has that signature and many will but it's true signature is the following: ```typescript -createDerived(derivedStateDefinition: DerivedStateDefinition): DerivedUserState +{ActiveUser|SingleUser|Global}State { + // ... rest of type left out for brevity + update(updateState: (state: T, dependency: TCombine) => T, options?: StateUpdateOptions); +} + +type StateUpdateOptions = { + shouldUpdate?: (state: T, dependency: TCombine) => boolean; + combineLatestWith?: Observable; + msTimeout?: number +} +``` + +The `shouldUpdate` option can be useful to help avoid an unnecessary update, and therefore avoid an +unnecessary emission of `state$`. You might want to use this to avoid setting state to `null` when +it is already `null`. The `shouldUpdate` method gives you in it's first parameter the value of state +before any change has been made to it and the dependency you have, optionally, provided through +`combineLatestWith`. To avoid setting `null` twice you could call `update` like below: + +```typescript +await myUserState.update(() => null, { shouldUpdate: (state) => state != null }); ``` -and the definition of `DerivedStateDefinition` looks like: +The `combineLatestWith` option can be useful when updates to your state depend on the data from +another stream of data. In +[this example](https://github.com/bitwarden/clients/blob/2eebf890b5b1cfbf5cb7d1395ed921897d0417fd/libs/common/src/auth/services/account.service.ts#L88-L107) +you can see how we don't want to set a user id to the active account id unless that user id exists +in our known accounts list. This can be preferred over the more manual implementation like such: ```typescript -class DerivedStateDefinition { - keyDefinition: KeyDefinition; - converter: (data: TFrom, context: { activeUserKey: UserKey, encryptService: EncryptService}) => Promise +const accounts = await firstValueFrom(this.accounts$); +if (accounts?.[userId] == null) { + throw new Error(); } +await this.activeAccountIdState.update(() => userId); ``` -This class encapsulates the logic for how to go from one version of your state into another form. This is often because encrypted data is stored on disk, but decrypted data is what will be shown to the user. +The use of the `combineLatestWith` option is preferred because it fixes a couple subtle issues. +First, the use of `firstValueFrom` with no `timeout`. Behind the scenes we enforce that the +observable given to `combineLatestWith` will emit a value in a timely manner, in this case a +`1000ms` timeout but that number is configurable through the `msTimeout` option. The second issue it +fixes, is that we don't guarantee that your `updateState` function is called the instant that the +`update` method is called. We do however promise that it will be called before the returned promise +resolves or rejects. This may be because we have a lock on the current storage key. No such locking +mechanism exists today but it may be implemented in the future. As such, it is safer to use +`combineLatestWith` because the data is more likely to retrieved closer to when it needs to be +evaluated. + +## FAQ From cd5c421a22b2bbd2bc1e45a99bf4ab5efe1cf144 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Wed, 6 Dec 2023 12:28:24 -0500 Subject: [PATCH 03/20] Apply Suggestions from Matt Co-authored-by: Matt Gibson --- docs/architecture/deep-dives/state/index.md | 38 +++++++++------------ 1 file changed, 17 insertions(+), 21 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 7a54785f..c539e18d 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -18,7 +18,7 @@ Core API's: ### `StateDefinition` `StateDefinition` is a simple API but a very core part of making the State Provider Framework work -smoothly. Teams will interact with it only in a single, `state-definitions.ts`, file in the +smoothly. It defines a storage location and top-level namespace for storage. Teams will interact with it only in a single, `state-definitions.ts`, file in the [`clients`](https://github.com/bitwarden/clients) repository. This file is located under platform code ownership but teams are expected to create edits to it. A team will edit this file to include a line like such: @@ -77,9 +77,8 @@ const MY_DOMAIN_DATA: KeyDefinition> = ``` The first argument to `KeyDefinition` is always the `StateDefinition` that this key should belong -to. The second argument should be a human readable, camelCase formatted, name of the -`KeyDefinition`. This name should be unique amongst all other `KeyDefinition`s that consume the same -`StateDefinition`. The responsibility of this uniqueness is on the team. As such, you should only +to. The second argument should be a human readable, camelCase formatted name of the +`KeyDefinition`. For example, the accounts service may wish to store a known accounts array on disk and choose `knownAccounts` to be the second argument. This name should be unique amongst all other `KeyDefinition`s that consume the same `StateDefinition`. The responsibility of this uniqueness is on the team. As such, you should only consume the `StateDefinition` of another team in your own `KeyDefinition` very rarely and if it is used, you should practice extreme caution, explicit comments stating such, and with coordination with the team that owns the `StateDefinition`. @@ -135,7 +134,7 @@ class FolderService { `ActiveUserState` is an object to help you maintain and view the state of the currently active user. If the currently active user changes, like through account switching. The data this object represents will change along with it. Gone is the need to subscribe to -`StateService.activeAccountUnlocked$`. You can see the most likely type definition of the API's on +`StateService.activeAccountUnlocked$`. You can see the type definition of the API on `ActiveUserState` below: ```typescript @@ -147,8 +146,7 @@ interface ActiveUserState { :::note -The definition of `update` shown above is not complete and the full definition includes more -customization that you can view in the [Advanced Usage](#advanced-usage) section. +Specifics around `StateUpdateOptions` are discussed in the [Advanced Usage](#advanced-usage) section. ::: @@ -156,17 +154,19 @@ The `update` method takes a function `updateState: (state: T) => T` that can be state in both a destructive and additive way. The function gives you a representation of what is currently saved as your state and it requires you to return the state that you want saved into storage. This means if you have an array on your state, you can `push` onto the array and return the -array back. +array back. The return value of the `updateState` function is always used as the new state value -- do not rely on object mutation to update! -The `state$` property provides you an `Observable` that can be subscribed to. +The `state$` property provides you with an `Observable` that can be subscribed to. `ActiveUserState.state$` will emit for the following reasons: - The active user changes. -- You call the `update` method. -- Another service in a different context calls `update` on their own instance of +- The chosen storage location emits an update to the key defined by `KeyDefinition`. This can occur for any reason including: + - You caused an update through the `update` method. + - Another service in a different context calls `update` on their own instance of `ActiveUserState` made from the same `KeyDefinition`. -- A `SingleUserState` method pointing at the same `KeyDefinition` as `ActiveUserState` and + - A `SingleUserState` method pointing at the same `KeyDefinition` as `ActiveUserState` and pointing at the user that is active that had `update` called + - Someone updates the key directly on the underlying storage service (Please don't do this) ### `GlobalState` @@ -177,11 +177,9 @@ when the `update` method is called, in this context, or another. ### `SingleUserState` `SingleUserState` behaves very similarly to `GlobalState` where neither will react to active -user changes and you instead give it the user you want it to care about up front. Because of that -`SingleUserState` exposes a `userId` property so you can see the user this instance is about. It -also has an interesting way it can interact with `ActiveUserState`, if you have pointed -`SingleUserState` at user who happens to be active at the time calling `update` from either of -those services will cause an emission from both of their `state$` observables. +user changes and you instead give it the user you want it to care about up front, which is publicly exposed as a `readonly` member. + +Updates to `SingleUserState` or `ActiveUserState` handling the same `KeyDefinition` will cause eachother to emit on their `state$` observables if the `userId` handled by the `SingleUserState` happens to be active at the time of the update. ## Migrating @@ -243,7 +241,7 @@ _TODO: Include PR's_ ## Testing Testing business logic with data and observables can sometimes be cumbersome, to help make that a -little easier, there are a sweet of helpful "fakes" that can be used instead of traditional "mocks". +little easier, there are a suite of helpful "fakes" that can be used instead of traditional "mocks". Now instead of calling `mock()` into your service you can instead use `new FakeStateProvider()`. @@ -253,9 +251,7 @@ _TODO: Refine user story_ ### `update` -It was mentioned earlier that update has the type signature -`update(updateState: (state: T) => T): Promise` but that is not entirely true. You can treat it -as if it has that signature and many will but it's true signature is the following: +The update method has options defined as follows: ```typescript {ActiveUser|SingleUser|Global}State { From e294c2f1d865b9e90bded0f3dd439c8c73a938d6 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Wed, 6 Dec 2023 13:34:25 -0500 Subject: [PATCH 04/20] Update docs/architecture/deep-dives/state/index.md Co-authored-by: Matt Gibson --- docs/architecture/deep-dives/state/index.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index c539e18d..02078509 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -172,7 +172,7 @@ The `state$` property provides you with an `Observable` that can be subscribe `GlobalState` has an incredibly similar API surface as `ActiveUserState` except it targets global scoped storage and does not emit an update to `state$` when the active user changes, only -when the `update` method is called, in this context, or another. +when the stored value is updated. ### `SingleUserState` From 942dd0e49c025fa9bec670f86404c2ce24274d3b Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Wed, 6 Dec 2023 16:53:51 -0500 Subject: [PATCH 05/20] More Feedback --- docs/architecture/deep-dives/state/index.md | 61 +++++++++++++++------ 1 file changed, 43 insertions(+), 18 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 02078509..45b318a7 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -8,17 +8,21 @@ Core API's: ## API's -- [`StateDefinition`](#statedefinition) -- [`KeyDefinition`](#keydefinition) +- [Storage Definitions](#storage-definitions) + - [`StateDefinition`](#statedefinition) + - [`KeyDefinition`](#keydefinition) - [`StateProvider`](#stateprovider) - [`ActiveUserState`](#activeuserstatet) - [`GlobalState`](#globalstatet) - [`SingleUserState`](#singleuserstatet) -### `StateDefinition` +### Storage Definitions + +#### `StateDefinition` `StateDefinition` is a simple API but a very core part of making the State Provider Framework work -smoothly. It defines a storage location and top-level namespace for storage. Teams will interact with it only in a single, `state-definitions.ts`, file in the +smoothly. It defines a storage location and top-level namespace for storage. Teams will interact +with it only in a single, `state-definitions.ts`, file in the [`clients`](https://github.com/bitwarden/clients) repository. This file is located under platform code ownership but teams are expected to create edits to it. A team will edit this file to include a line like such: @@ -47,7 +51,7 @@ The Platform team will weigh the possibility of supporting secure storage down t ::: -### `KeyDefinition` +#### `KeyDefinition` `KeyDefinition` builds on the idea of [`StateDefinition`](#statedefinition) but it gets more specific about the data to be stored. `KeyDefinition`s can also be instantiated in your own teams @@ -77,11 +81,26 @@ const MY_DOMAIN_DATA: KeyDefinition> = ``` The first argument to `KeyDefinition` is always the `StateDefinition` that this key should belong -to. The second argument should be a human readable, camelCase formatted name of the -`KeyDefinition`. For example, the accounts service may wish to store a known accounts array on disk and choose `knownAccounts` to be the second argument. This name should be unique amongst all other `KeyDefinition`s that consume the same `StateDefinition`. The responsibility of this uniqueness is on the team. As such, you should only -consume the `StateDefinition` of another team in your own `KeyDefinition` very rarely and if it is -used, you should practice extreme caution, explicit comments stating such, and with coordination -with the team that owns the `StateDefinition`. +to. The second argument should be a human readable, camelCase formatted name of the `KeyDefinition`. +For example, the accounts service may wish to store a known accounts array on disk and choose +`knownAccounts` to be the second argument. This name should be unique amongst all other +`KeyDefinition`s that consume the same `StateDefinition`. The responsibility of this uniqueness is +on the team. As such, you should never consume the `StateDefinition` of another team in your own +`KeyDefinition`. The third argument is an object of type +[`KeyDefinitionOptions`](#keydefinitionoptions). + +##### `KeyDefinitionOptions` + +`deserializer` (required) - Takes a method that gives you your state in it's JSON format and makes +you responsible for converting that into JSON back into a full JavaScript object, if you choose to +use a class to represent your state that means having it's prototype and any method you declare on +it. If your state is a simple value like `string`, `boolean`, `number`, or arrays of those values, +your deserializer can be as simple as `data => data`. But, if your data has something like `Date`, +which gets serialized as a string you will need to convert that back into a `Date` like: +`data => new Date(data)`. + +`cleanupDelayMs` (optional) - Takes a number of milliseconds to wait before cleaning up the state +after the last subscriber has unsubscribed. Defaults to 1000ms. ### `StateProvider` @@ -129,7 +148,7 @@ class FolderService { } ``` -### ActiveUserState\ +### `ActiveUserState` `ActiveUserState` is an object to help you maintain and view the state of the currently active user. If the currently active user changes, like through account switching. The data this object @@ -146,7 +165,8 @@ interface ActiveUserState { :::note -Specifics around `StateUpdateOptions` are discussed in the [Advanced Usage](#advanced-usage) section. +Specifics around `StateUpdateOptions` are discussed in the [Advanced Usage](#advanced-usage) +section. ::: @@ -154,18 +174,20 @@ The `update` method takes a function `updateState: (state: T) => T` that can be state in both a destructive and additive way. The function gives you a representation of what is currently saved as your state and it requires you to return the state that you want saved into storage. This means if you have an array on your state, you can `push` onto the array and return the -array back. The return value of the `updateState` function is always used as the new state value -- do not rely on object mutation to update! +array back. The return value of the `updateState` function is always used as the new state value -- +do not rely on object mutation to update! The `state$` property provides you with an `Observable` that can be subscribed to. `ActiveUserState.state$` will emit for the following reasons: - The active user changes. -- The chosen storage location emits an update to the key defined by `KeyDefinition`. This can occur for any reason including: +- The chosen storage location emits an update to the key defined by `KeyDefinition`. This can occur + for any reason including: - You caused an update through the `update` method. - Another service in a different context calls `update` on their own instance of - `ActiveUserState` made from the same `KeyDefinition`. + `ActiveUserState` made from the same `KeyDefinition`. - A `SingleUserState` method pointing at the same `KeyDefinition` as `ActiveUserState` and - pointing at the user that is active that had `update` called + pointing at the user that is active that had `update` called - Someone updates the key directly on the underlying storage service (Please don't do this) ### `GlobalState` @@ -177,9 +199,12 @@ when the stored value is updated. ### `SingleUserState` `SingleUserState` behaves very similarly to `GlobalState` where neither will react to active -user changes and you instead give it the user you want it to care about up front, which is publicly exposed as a `readonly` member. +user changes and you instead give it the user you want it to care about up front, which is publicly +exposed as a `readonly` member. -Updates to `SingleUserState` or `ActiveUserState` handling the same `KeyDefinition` will cause eachother to emit on their `state$` observables if the `userId` handled by the `SingleUserState` happens to be active at the time of the update. +Updates to `SingleUserState` or `ActiveUserState` handling the same `KeyDefinition` will cause each +other to emit on their `state$` observables if the `userId` handled by the `SingleUserState` happens +to be active at the time of the update. ## Migrating From 9401adc6c533a2443ec1723b4a9af9f17c584e2f Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 7 Dec 2023 10:11:31 -0500 Subject: [PATCH 06/20] Refine --- docs/architecture/deep-dives/state/index.md | 29 ++++++++++++++++----- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 45b318a7..b3ac3200 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -18,6 +18,21 @@ Core API's: ### Storage Definitions +In order to store and retrieve data, we need to have constant keys to reference storage locations. +This includes a storage medium (disk or memory), and a unique key. `StateDefinition` and +`KeyDefinition` classes that allow for reasonable reuse of partial namespaces while allowing +expansion to precise keys. They exist to help minimize the potential of overlaps in a distributed +storage framework. + +:::warning + +Once you have created the definitions, you need to take extreme caution when changing any part of +the namespace. If you change the name of a `StateDefinition` pointing at `"disk"` without also +migrating data from the old name to the new name you will lose data. Data pointing at `"memory"` can +have their name changed. + +::: + #### `StateDefinition` `StateDefinition` is a simple API but a very core part of making the State Provider Framework work @@ -45,9 +60,10 @@ _TODO: Make tests_ :::note -Secure storage is not currently supported as a storage location in the State Provider Framework. If -you need to store data in the secure storage implementations, please continue to use `StateService`. -The Platform team will weigh the possibility of supporting secure storage down the road. +Secure storage is not currently supported as a storage location in the State Provider Framework. For +now, don't migrate data that is stored in secure storage but please contact platform when you have +data you wanted to migrate so we can prioritize a long term solution. If you need new data in secure +storage, use `StateService` for now. ::: @@ -159,7 +175,7 @@ represents will change along with it. Gone is the need to subscribe to ```typescript interface ActiveUserState { state$: Observable; - update(updateState: (state: T) => T): Promise; + update: (updateState: (state: T, dependency: TCombine) => T, stateUpdateOptions?: StateUpdateOptions): Promise; } ``` @@ -254,8 +270,9 @@ export class MoveToStateProvider extends Migrator<10, 11> { `getAccounts` only gets data from the legacy account object that was used in `StateService`. As data gets migrated off of that account object the response from `getAccounts`, which returns a record -where the key will be a users id and the value being the legacy account object. It can continue to -be used to retrieve the known authenticated user ids though. +where the key will be a users id and the value being the legacy account object. + +_TODO: Implement method just for user ids_ ::: From bf9548e23a3d856c1e10b6f863d3fe8c64e19495 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Fri, 8 Dec 2023 13:26:59 -0500 Subject: [PATCH 07/20] Add Diagram and FAQ --- .../deep-dives/state/State_Diagram.svg | 21 +++++++++++ docs/architecture/deep-dives/state/index.md | 35 +++++++++++++++++++ 2 files changed, 56 insertions(+) create mode 100644 docs/architecture/deep-dives/state/State_Diagram.svg diff --git a/docs/architecture/deep-dives/state/State_Diagram.svg b/docs/architecture/deep-dives/state/State_Diagram.svg new file mode 100644 index 00000000..0ba405b6 --- /dev/null +++ b/docs/architecture/deep-dives/state/State_Diagram.svg @@ -0,0 +1,21 @@ + + + + + + + + StateGlobalUserPlatform owned & Platform managedStateDefinitionPlatform owned & Team managedKeyDefinitionTeam owned & Team managedglobaluser_ac06d663-bbbc-4a51-a764-5d105ae6f7cbglobal_stateuser_ac06d663-bbbc-4a51-a764-5d105ae6f7cb_stateglobal_state_keyuser_ac06d663-bbbc-4a51-a764-5d105ae6f7cb_state_key \ No newline at end of file diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index b3ac3200..529da775 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -344,3 +344,38 @@ mechanism exists today but it may be implemented in the future. As such, it is s evaluated. ## FAQ + +### Do I need to have my own in memory cache? + +If you previously had a memory cache that exactly represented the data you stored on disk (not +decrypted for example), then you likely don't need that anymore. All the `*State` classes maintain +an in memory cache of the last known value in state for as long as someone is subscribed to the +data. The cache is cleared after 1000ms of no one subscribing to the state though. If you know you +have sporadic subscribers and a high cost of going to disk you may increase that time using the +`cleanupDelayMs` on `KeyDefinitionOptions`. + +### I store my data as a Record/Map but expose it as an array, what should I do? + +Give `KeyDefinition` generic the record shape you want, or even use the static `record` helper +method. Then to convert that to an array that you expose just do a simple +`.pipe(map(data => this.transform(data)))` to convert that to the array you want to expose. + +### Why KeyDefinitionLike + +`KeyDefinitionLike` exists to help you create a frozen in time version of your `KeyDefinition` this +is helpful in state migrations so that you don't have to import something from the greater +application which is something that should rarely happen. + +### When does my deserializer run? + +The `deserialier` that you provide in the `KeyDefinitionOptions` is used whenever your state is +retrieved from a storage service that stores it's data as JSON. All disk storage services serialize +data into JSON but memory storage differs in this area across platforms. That's why it's imperative +to include a high quality JSON deserializer even if you think your object will only be stored in +memory. This can mean you might be able to drop the `*Data` class pattern for your code. Since the +`*Data` class generally represented the JSON safe version of your state which we now do +automatically through the `Jsonify` given to your in your `deserializer` method. + +## Structure + +![State Diagram](State_Diagram.svg) From b9b14ea31ea0bcf396a91a94ad017369f6823f3c Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Tue, 12 Dec 2023 13:01:49 -0500 Subject: [PATCH 08/20] Apply suggestions from code review Co-authored-by: Matt Bishop --- docs/architecture/deep-dives/state/index.md | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 529da775..6d4b6698 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -4,9 +4,8 @@ The state provider framework was designed for the purpose of allowing state to b but also to enforce good practices, reduce boilerplate around account switching, and provide a trustworthy observable stream of that state. -Core API's: -## API's +## APIs - [Storage Definitions](#storage-definitions) - [`StateDefinition`](#statedefinition) @@ -16,7 +15,7 @@ Core API's: - [`GlobalState`](#globalstatet) - [`SingleUserState`](#singleuserstatet) -### Storage Definitions +### Storage definitions In order to store and retrieve data, we need to have constant keys to reference storage locations. This includes a storage medium (disk or memory), and a unique key. `StateDefinition` and @@ -289,7 +288,7 @@ Now instead of calling `mock()` into your service you can instead _TODO: Refine user story_ -## Advanced Usage +## Advanced usage ### `update` From 8aac1c4d6a534cf8bdf2dfa9b544ed2590006a4d Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Tue, 12 Dec 2023 13:05:36 -0500 Subject: [PATCH 09/20] Run Prettier --- docs/architecture/deep-dives/state/index.md | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 6d4b6698..d0369a0d 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -4,10 +4,9 @@ The state provider framework was designed for the purpose of allowing state to b but also to enforce good practices, reduce boilerplate around account switching, and provide a trustworthy observable stream of that state. - ## APIs -- [Storage Definitions](#storage-definitions) +- [Storage definitions](#storage-definitions) - [`StateDefinition`](#statedefinition) - [`KeyDefinition`](#keydefinition) - [`StateProvider`](#stateprovider) From ed1f1f8e3ed9ac2140f526517b5cb42e868253a0 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 4 Jan 2024 16:14:18 -0500 Subject: [PATCH 10/20] Stub Out `deserializer` and add comment Co-authored-by: audrey-jensen --- docs/architecture/deep-dives/state/index.md | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index d0369a0d..96e62fed 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -75,7 +75,8 @@ example instantiations are: ```typescript const MY_DOMAIN_DATA = new KeyDefinition(MY_DOMAIN_DISK, "data", { - deserializer: (jsonData) => null, // convert to your data from json + // convert to your data from serialized representation `{ foo: string }` to fully-typed `MyState` + deserializer: (jsonData) => MyState.fromJSON(jsonData), }); // Or if your state is an array, use the built in helper @@ -83,14 +84,14 @@ const MY_DOMAIN_DATA: KeyDefinition = KeyDefinition.array null, // provide a deserializer just for the element of the array + deserializer: (jsonDataElement) => MyState.fromJSON(jsonDataElement), // provide a deserializer just for the element of the array }, ); // record const MY_DOMAIN_DATA: KeyDefinition> = KeyDefinition.record(MY_DOMAIN_DISK, "data", { - deserializer: (jsonDataValue) => null, // provide a deserializer just for the value in each key-value pair + deserializer: (jsonDataValue) => MyState.fromJSON(jsonDataValue), // provide a deserializer just for the value in each key-valu pair }); ``` From e907ccb3b1b113adc7725835e393698e085b2e7d Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 4 Jan 2024 16:17:21 -0500 Subject: [PATCH 11/20] Clarify Wording --- docs/architecture/deep-dives/state/index.md | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 96e62fed..ede77dd3 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -121,8 +121,10 @@ after the last subscriber has unsubscribed. Defaults to 1000ms. `StateProvider` is an injectable service that includes 3 methods for getting state. These three methods are helpers for invoking their more modular siblings `ActiveStateProvider.get`, -`SingleUserStateProvider.get`, and `GlobalStateProvider.get` and they have the following type -definitions: +`SingleUserStateProvider.get`, and `GlobalStateProvider.get`, these siblings can all be injected +into your service as well. If you prefer thin dependencies over the slightly larger changeset +required, you can absolutely make use of the more targeted providers. `StateProvider` has the +following type definition (aliasing the targeted providers): ```typescript interface StateProvider { @@ -311,7 +313,8 @@ The `shouldUpdate` option can be useful to help avoid an unnecessary update, and unnecessary emission of `state$`. You might want to use this to avoid setting state to `null` when it is already `null`. The `shouldUpdate` method gives you in it's first parameter the value of state before any change has been made to it and the dependency you have, optionally, provided through -`combineLatestWith`. To avoid setting `null` twice you could call `update` like below: +`combineLatestWith`. To avoid setting `null` to your state when it's already `null` you could call +`update` like below: ```typescript await myUserState.update(() => null, { shouldUpdate: (state) => state != null }); From 7b84808ad7e51132636414504cd44e4094e104cd Mon Sep 17 00:00:00 2001 From: Matt Gibson Date: Wed, 24 Jan 2024 14:22:18 -0500 Subject: [PATCH 12/20] First pass at derived state documentation (#269) * First pass at derived state documentation * `npm run prettier` :robot: * Apply suggestions from code review Co-authored-by: Matt Bishop --------- Co-authored-by: Matt Bishop --- .../deep-dives/state/derived-state.md | 129 ++++++++++++++++++ docs/architecture/deep-dives/state/index.md | 15 +- 2 files changed, 139 insertions(+), 5 deletions(-) create mode 100644 docs/architecture/deep-dives/state/derived-state.md diff --git a/docs/architecture/deep-dives/state/derived-state.md b/docs/architecture/deep-dives/state/derived-state.md new file mode 100644 index 00000000..bb187310 --- /dev/null +++ b/docs/architecture/deep-dives/state/derived-state.md @@ -0,0 +1,129 @@ +# Derived State + +It is common to need to cache the result of expensive work that does not represent true alterations +in application state. Derived state exists to store this kind of data in memory and keep it up to +date when the underlying observable state changes. + +## `DeriveDefinition` + +Derived state has all of the same issues with storage and retrieval that normal state does. Similar +to `KeyDefinition`, derived state depends on `DeriveDefinition`s to define magic string keys to +store and retrieve data from a cache. Unlike normal state, derived state is always stored in memory. +It still takes a `StateDefinition`, but this is used only to define a namespace for the derived +state, the storage location is ignored. _This can lead to collisions if you use the same key for two +different derived state definitions in the same namespace._ + +Derive definitions can be created in two ways: + + + +```typescript +new DeriveDefinition(STATE_DEFINITION, "uniqueKey", _DeriveOptions_); + +// or + +const keyDefinition: KeyDefinition; +DeriveDefinition.from(keyDefinition, _DeriveOptions_); +``` + +The first allows building from basic building blocks, the second recognizes that derived state is +often built from existing state and allows you to create a definition from an existing +`KeyDefinition`. The resulting `DeriveDefinition` will have the same state namespace, key, and +`TFrom` type as the `KeyDefinition` it was built from. + +### Type Parameters + +`DeriveDefinition`s have three type parameters: + +- `TFrom`: The type of the state that the derived state is built from. +- `TTo`: The type of the derived state. +- `TDeps`: defines the dependencies required to derive the state. This is further discussed in + [Derive Definition Options](#derivedefinitionoptions). + +### `DeriveDefinitionOptions` + +[The `DeriveDefinition` section](#deriveDefinitionFactories) specifies a third parameter as +`_DeriveOptions_`, which is used to fully specify the way to transform `TFrom` to `TTo`. + +- `deserializer` - For the same reasons as [Key Definition Options](#keydefinitionoptions), + `DeriveDefinition`s require have a `deserializer` function that is used to convert the stored data + back into the `TTo` type. +- `derive` - A function that takes the current state and returns the derived state. This function + takes two parameters: + - `from` - The latest value of the parent state. + - `deps` - dependencies used to instantiate the derived state. These are provided when the + `DerivedState` class is instantiated. This object should contain all of the application runtime + dependencies for transform the from parent state to the derived state. +- `cleanupDelayMs` (optional) - Takes the number of milliseconds to wait before cleaning up the + state after the last subscriber unsubscribes. Defaults to 1000ms. If you have a particularly + expensive operation, such as decryption of a vault, it may be worth increasing this value to avoid + unnecessary recomputation. + +Specifying dependencies required for your `derive` function is done through the type parameters on +`DerivedState`. + +```typescript +new DerivedState(); +``` + +would require a `deps` object with an `example` property of type `Dependency` to be passed to any +`DerivedState` configured to use the `DerivedDefinition`. + +:::warning + +Both `derive` and `deserializer` functions should take null inputs into consideration. Both parent +state and stored data for deserialization can be `null` or `undefined`. + +::: + +## `DerivedStateProvider` + +The `DerivedState` class has a purpose-built provider which instantiates the +correct `DerivedState` implementation for a given application context. These derived states are +cached within a context, so that multiple instances of the same derived state will share the same +underlying cache, based on the `DeriveDefinition` used to create them. + +Instantiating a `DerivedState` instance requires an observable parent state, the derive definition, +and an object containing the dependencies defined in the `DeriveDefinition` type parameters. + +```typescript +interface DerivedStateProvider { + get: ( + parentState$: Observable, + deriveDefinition: DeriveDefinition, + dependencies: TDeps, + ) => DerivedState; +} +``` + +:::tip + +Any observable can be used as the parent state. If you need to perform some kind of work on data +stored to disk prior to sending to your `derive` functions, that is supported. + +::: + +## `DerivedState` + +`DerivedState` is intended to be built with a provider rather than directly instantiated. The +interface consists of two items: + +```typescript +interface DerivedState { + state$: Observable; + forceValue(value: T): Promise; +} +``` + +- `state$` - An observable that emits the current value of the derived state and emits new values + whenever the parent state changes. +- `forceValue` - A function that takes a value and immediately sets `state$` to that value. This is + useful for clearing derived state from memory without impacting the parent state, such as during + logout. + +:::note + +`forceValue` forces `state$` _once_. It does not prevent the derived state from being recomputed +when the parent state changes. + +::: diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index ede77dd3..2d2eb05d 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -119,18 +119,23 @@ after the last subscriber has unsubscribed. Defaults to 1000ms. ### `StateProvider` -`StateProvider` is an injectable service that includes 3 methods for getting state. These three +`StateProvider` is an injectable service that includes 4 methods for getting state. These four methods are helpers for invoking their more modular siblings `ActiveStateProvider.get`, -`SingleUserStateProvider.get`, and `GlobalStateProvider.get`, these siblings can all be injected -into your service as well. If you prefer thin dependencies over the slightly larger changeset -required, you can absolutely make use of the more targeted providers. `StateProvider` has the -following type definition (aliasing the targeted providers): +`SingleUserStateProvider.get`, `GlobalStateProvider.get`, and `DerivedStateProvider`, these siblings +can all be injected into your service as well. If you prefer thin dependencies over the slightly +larger changeset required, you can absolutely make use of the more targeted providers. +`StateProvider` has the following type definition (aliasing the targeted providers): ```typescript interface StateProvider { getActive(keyDefinition: KeyDefinition): ActiveUserState; getUser(userId: UserId, keyDefinition: KeyDefinition): SingleUserState; getGlobal(keyDefinition: KeyDefinition): GlobalState; + getDerived( + parentState$: Observable, + deriveDefinition: DeriveDefinition, + dependenciess: TDeps, + ); } ``` From 04c127b6a8369bbe771a379cca78a436b2038c1d Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 29 Jan 2024 13:07:18 -0500 Subject: [PATCH 13/20] Apply suggestions from code review Co-authored-by: Matt Bishop --- docs/architecture/deep-dives/state/index.md | 66 ++++++++++----------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 2d2eb05d..da4d8166 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -17,17 +17,17 @@ trustworthy observable stream of that state. ### Storage definitions In order to store and retrieve data, we need to have constant keys to reference storage locations. -This includes a storage medium (disk or memory), and a unique key. `StateDefinition` and -`KeyDefinition` classes that allow for reasonable reuse of partial namespaces while allowing +This includes a storage medium (disk or memory) and a unique key. `StateDefinition` and +`KeyDefinition` classes allow for reasonable reuse of partial namespaces while also enabling expansion to precise keys. They exist to help minimize the potential of overlaps in a distributed storage framework. :::warning -Once you have created the definitions, you need to take extreme caution when changing any part of +Once you have created the definitions you need to take extreme caution when changing any part of the namespace. If you change the name of a `StateDefinition` pointing at `"disk"` without also migrating data from the old name to the new name you will lose data. Data pointing at `"memory"` can -have their name changed. +have its name changed. ::: @@ -35,21 +35,21 @@ have their name changed. `StateDefinition` is a simple API but a very core part of making the State Provider Framework work smoothly. It defines a storage location and top-level namespace for storage. Teams will interact -with it only in a single, `state-definitions.ts`, file in the -[`clients`](https://github.com/bitwarden/clients) repository. This file is located under platform +with it only in a single `state-definitions.ts` file in the +[`clients`](https://github.com/bitwarden/clients) repository. This file is located under Platform team code ownership but teams are expected to create edits to it. A team will edit this file to include a -line like such: +line such as: ```typescript export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk"); ``` The first argument to the `StateDefinition` constructor is expected to be a human readable, -camelCase formatted name for your domain, or state area. The second argument will either be the +camelCase-formatted name for your domain or state area. The second argument will either be the string literal `"disk"` or `"memory"` dictating where all the state using this `StateDefinition` -should be stored. The platform team will be responsible to reviewing all new and updated entries in +should be stored. The Platform team will be responsible to reviewing all new and updated entries in this file and will be looking to make sure that there are no duplicate entries containing the same -state name and state location. Teams CAN have the same state name used for both `"disk"` and +state name and state location. Teams _can_ have the same state name used for both `"disk"` and `"memory"` locations. Tests are included to ensure this uniqueness and core naming guidelines so you can ensure a review for a new `StateDefinition` entry can be done promptly and with very few surprises. @@ -59,8 +59,8 @@ _TODO: Make tests_ :::note Secure storage is not currently supported as a storage location in the State Provider Framework. For -now, don't migrate data that is stored in secure storage but please contact platform when you have -data you wanted to migrate so we can prioritize a long term solution. If you need new data in secure +now, don't migrate data that is stored in secure storage but please contact the Platform team when you have +data you wanted to migrate so we can prioritize a long-term solution. If you need new data in secure storage, use `StateService` for now. ::: @@ -68,7 +68,7 @@ storage, use `StateService` for now. #### `KeyDefinition` `KeyDefinition` builds on the idea of [`StateDefinition`](#statedefinition) but it gets more -specific about the data to be stored. `KeyDefinition`s can also be instantiated in your own teams +specific about the data to be stored. `KeyDefinition`s can also be instantiated in your own team's code. This might mean creating it in the same file as the service you plan to consume it or you may want to have a single `key-definitions.ts` file that contains all the entries for your team. Some example instantiations are: @@ -79,7 +79,7 @@ const MY_DOMAIN_DATA = new KeyDefinition(MY_DOMAIN_DISK, "data", { deserializer: (jsonData) => MyState.fromJSON(jsonData), }); -// Or if your state is an array, use the built in helper +// Or if your state is an array, use the built-in helper const MY_DOMAIN_DATA: KeyDefinition = KeyDefinition.array( MY_DOMAIN_DISK, "data", @@ -91,12 +91,12 @@ const MY_DOMAIN_DATA: KeyDefinition = KeyDefinition.array> = KeyDefinition.record(MY_DOMAIN_DISK, "data", { - deserializer: (jsonDataValue) => MyState.fromJSON(jsonDataValue), // provide a deserializer just for the value in each key-valu pair + deserializer: (jsonDataValue) => MyState.fromJSON(jsonDataValue), // provide a deserializer just for the value in each key-value pair }); ``` The first argument to `KeyDefinition` is always the `StateDefinition` that this key should belong -to. The second argument should be a human readable, camelCase formatted name of the `KeyDefinition`. +to. The second argument should be a human readable, camelCase-formatted name of the `KeyDefinition`. For example, the accounts service may wish to store a known accounts array on disk and choose `knownAccounts` to be the second argument. This name should be unique amongst all other `KeyDefinition`s that consume the same `StateDefinition`. The responsibility of this uniqueness is @@ -108,7 +108,7 @@ on the team. As such, you should never consume the `StateDefinition` of another `deserializer` (required) - Takes a method that gives you your state in it's JSON format and makes you responsible for converting that into JSON back into a full JavaScript object, if you choose to -use a class to represent your state that means having it's prototype and any method you declare on +use a class to represent your state that means having its prototype and any method you declare on it. If your state is a simple value like `string`, `boolean`, `number`, or arrays of those values, your deserializer can be as simple as `data => data`. But, if your data has something like `Date`, which gets serialized as a string you will need to convert that back into a `Date` like: @@ -121,7 +121,7 @@ after the last subscriber has unsubscribed. Defaults to 1000ms. `StateProvider` is an injectable service that includes 4 methods for getting state. These four methods are helpers for invoking their more modular siblings `ActiveStateProvider.get`, -`SingleUserStateProvider.get`, `GlobalStateProvider.get`, and `DerivedStateProvider`, these siblings +`SingleUserStateProvider.get`, `GlobalStateProvider.get`, and `DerivedStateProvider`. These siblings can all be injected into your service as well. If you prefer thin dependencies over the slightly larger changeset required, you can absolutely make use of the more targeted providers. `StateProvider` has the following type definition (aliasing the targeted providers): @@ -139,9 +139,9 @@ interface StateProvider { } ``` -A very common practice will be to inject `StateProvider` in your services constructor and call +A very common practice will be to inject `StateProvider` in your service's constructor and call `getActive`, `getGlobal`, or both in your constructor and then store private properties for the -resulting `ActiveUserState` and/or `GlobalState`. It's less common to need to call `getUser` +resulting `ActiveUserState` and / or `GlobalState`. It's less common to need to call `getUser` in the constructor because it will require you to know the `UserId` of the user you are attempting to edit. Instead you will add `private` to the constructor argument injecting `StateProvider` and instead use it in a method like in the below example. @@ -173,7 +173,7 @@ class FolderService { ### `ActiveUserState` `ActiveUserState` is an object to help you maintain and view the state of the currently active -user. If the currently active user changes, like through account switching. The data this object +user. If the currently active user changes, like through account switching, the data this object represents will change along with it. Gone is the need to subscribe to `StateService.activeAccountUnlocked$`. You can see the type definition of the API on `ActiveUserState` below: @@ -210,12 +210,12 @@ The `state$` property provides you with an `Observable` that can be subscribe `ActiveUserState` made from the same `KeyDefinition`. - A `SingleUserState` method pointing at the same `KeyDefinition` as `ActiveUserState` and pointing at the user that is active that had `update` called - - Someone updates the key directly on the underlying storage service (Please don't do this) + - Someone updates the key directly on the underlying storage service _(please don't do this)_ ### `GlobalState` `GlobalState` has an incredibly similar API surface as `ActiveUserState` except it targets -global scoped storage and does not emit an update to `state$` when the active user changes, only +global-scoped storage and does not emit an update to `state$` when the active user changes, only when the stored value is updated. ### `SingleUserState` @@ -276,7 +276,7 @@ export class MoveToStateProvider extends Migrator<10, 11> { `getAccounts` only gets data from the legacy account object that was used in `StateService`. As data gets migrated off of that account object the response from `getAccounts`, which returns a record -where the key will be a users id and the value being the legacy account object. +where the key will be a user's ID and the value being the legacy account object. _TODO: Implement method just for user ids_ @@ -288,8 +288,8 @@ _TODO: Include PR's_ ## Testing -Testing business logic with data and observables can sometimes be cumbersome, to help make that a -little easier, there are a suite of helpful "fakes" that can be used instead of traditional "mocks". +Testing business logic with data and observables can sometimes be cumbersome. To help make that a +little easier there are a suite of helpful "fakes" that can be used instead of traditional "mocks". Now instead of calling `mock()` into your service you can instead use `new FakeStateProvider()`. @@ -316,7 +316,7 @@ type StateUpdateOptions = { The `shouldUpdate` option can be useful to help avoid an unnecessary update, and therefore avoid an unnecessary emission of `state$`. You might want to use this to avoid setting state to `null` when -it is already `null`. The `shouldUpdate` method gives you in it's first parameter the value of state +it is already `null`. The `shouldUpdate` method gives you in its first parameter the value of state before any change has been made to it and the dependency you have, optionally, provided through `combineLatestWith`. To avoid setting `null` to your state when it's already `null` you could call `update` like below: @@ -328,7 +328,7 @@ await myUserState.update(() => null, { shouldUpdate: (state) => state != null }) The `combineLatestWith` option can be useful when updates to your state depend on the data from another stream of data. In [this example](https://github.com/bitwarden/clients/blob/2eebf890b5b1cfbf5cb7d1395ed921897d0417fd/libs/common/src/auth/services/account.service.ts#L88-L107) -you can see how we don't want to set a user id to the active account id unless that user id exists +you can see how we don't want to set a user ID to the active account ID unless that user ID exists in our known accounts list. This can be preferred over the more manual implementation like such: ```typescript @@ -343,7 +343,7 @@ The use of the `combineLatestWith` option is preferred because it fixes a couple First, the use of `firstValueFrom` with no `timeout`. Behind the scenes we enforce that the observable given to `combineLatestWith` will emit a value in a timely manner, in this case a `1000ms` timeout but that number is configurable through the `msTimeout` option. The second issue it -fixes, is that we don't guarantee that your `updateState` function is called the instant that the +fixes is that we don't guarantee that your `updateState` function is called the instant that the `update` method is called. We do however promise that it will be called before the returned promise resolves or rejects. This may be because we have a lock on the current storage key. No such locking mechanism exists today but it may be implemented in the future. As such, it is safer to use @@ -352,7 +352,7 @@ evaluated. ## FAQ -### Do I need to have my own in memory cache? +### Do I need to have my own in-memory cache? If you previously had a memory cache that exactly represented the data you stored on disk (not decrypted for example), then you likely don't need that anymore. All the `*State` classes maintain @@ -361,7 +361,7 @@ data. The cache is cleared after 1000ms of no one subscribing to the state thoug have sporadic subscribers and a high cost of going to disk you may increase that time using the `cleanupDelayMs` on `KeyDefinitionOptions`. -### I store my data as a Record/Map but expose it as an array, what should I do? +### I store my data as a Record / Map but expose it as an array -- what should I do? Give `KeyDefinition` generic the record shape you want, or even use the static `record` helper method. Then to convert that to an array that you expose just do a simple @@ -369,14 +369,14 @@ method. Then to convert that to an array that you expose just do a simple ### Why KeyDefinitionLike -`KeyDefinitionLike` exists to help you create a frozen in time version of your `KeyDefinition` this +`KeyDefinitionLike` exists to help you create a frozen-in-time version of your `KeyDefinition`. This is helpful in state migrations so that you don't have to import something from the greater application which is something that should rarely happen. ### When does my deserializer run? The `deserialier` that you provide in the `KeyDefinitionOptions` is used whenever your state is -retrieved from a storage service that stores it's data as JSON. All disk storage services serialize +retrieved from a storage service that stores its data as JSON. All disk storage services serialize data into JSON but memory storage differs in this area across platforms. That's why it's imperative to include a high quality JSON deserializer even if you think your object will only be stored in memory. This can mean you might be able to drop the `*Data` class pattern for your code. Since the From 46a8c8e995f8dff38b4215ecc0c6bc91e852bd3a Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 29 Jan 2024 15:26:42 -0500 Subject: [PATCH 14/20] Fill In TODOs --- docs/architecture/deep-dives/state/index.md | 35 +++++++++++---------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index da4d8166..9d569e02 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -24,10 +24,10 @@ storage framework. :::warning -Once you have created the definitions you need to take extreme caution when changing any part of -the namespace. If you change the name of a `StateDefinition` pointing at `"disk"` without also -migrating data from the old name to the new name you will lose data. Data pointing at `"memory"` can -have its name changed. +Once you have created the definitions you need to take extreme caution when changing any part of the +namespace. If you change the name of a `StateDefinition` pointing at `"disk"` without also migrating +data from the old name to the new name you will lose data. Data pointing at `"memory"` can have its +name changed. ::: @@ -36,9 +36,9 @@ have its name changed. `StateDefinition` is a simple API but a very core part of making the State Provider Framework work smoothly. It defines a storage location and top-level namespace for storage. Teams will interact with it only in a single `state-definitions.ts` file in the -[`clients`](https://github.com/bitwarden/clients) repository. This file is located under Platform team -code ownership but teams are expected to create edits to it. A team will edit this file to include a -line such as: +[`clients`](https://github.com/bitwarden/clients) repository. This file is located under Platform +team code ownership but teams are expected to create edits to it. A team will edit this file to +include a line such as: ```typescript export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk"); @@ -52,16 +52,16 @@ this file and will be looking to make sure that there are no duplicate entries c state name and state location. Teams _can_ have the same state name used for both `"disk"` and `"memory"` locations. Tests are included to ensure this uniqueness and core naming guidelines so you can ensure a review for a new `StateDefinition` entry can be done promptly and with very few -surprises. - -_TODO: Make tests_ +surprises. The tests can be seen +[here](https://github.com/bitwarden/clients/blob/main/libs/common/src/platform/state/state-definitions.spec.ts) +([permalink](https://github.com/bitwarden/clients/blob/daef7572bf19bc34be6cc661ff02c64692d3ad3e/libs/common/src/platform/state/state-definitions.spec.ts)). :::note Secure storage is not currently supported as a storage location in the State Provider Framework. For -now, don't migrate data that is stored in secure storage but please contact the Platform team when you have -data you wanted to migrate so we can prioritize a long-term solution. If you need new data in secure -storage, use `StateService` for now. +now, don't migrate data that is stored in secure storage but please contact the Platform team when +you have data you wanted to migrate so we can prioritize a long-term solution. If you need new data +in secure storage, use `StateService` for now. ::: @@ -278,13 +278,12 @@ export class MoveToStateProvider extends Migrator<10, 11> { gets migrated off of that account object the response from `getAccounts`, which returns a record where the key will be a user's ID and the value being the legacy account object. -_TODO: Implement method just for user ids_ - ::: ### Example PRs -_TODO: Include PR's_ +- [`EnvironmentService`](https://github.com/bitwarden/clients/pull/7621) +- [Org Keys](https://github.com/bitwarden/clients/pull/7521) ## Testing @@ -293,7 +292,9 @@ little easier there are a suite of helpful "fakes" that can be used instead of t Now instead of calling `mock()` into your service you can instead use `new FakeStateProvider()`. -_TODO: Refine user story_ +`FakeStateProvider` exposes the specific provider's fakes as properties on itself. Each of those +specific providers gives a method `getFake` that allows you to get the fake version of state that +you can control and `expect`. ## Advanced usage From 2c23f88ee98671c774774ba31071b8419d64ae2d Mon Sep 17 00:00:00 2001 From: Todd Martin <106564991+trmartin4@users.noreply.github.com> Date: Tue, 6 Feb 2024 12:35:12 -0500 Subject: [PATCH 15/20] Added optional constructor parameter and updated mapping instructions. (#296) * Added optional constructor parameter and updated mapping instructions. * Fixed table * Changed verbiage. * Updated verbiage. --- docs/architecture/deep-dives/state/index.md | 81 ++++++++++++++++----- 1 file changed, 62 insertions(+), 19 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index da4d8166..feb4329e 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -24,21 +24,30 @@ storage framework. :::warning -Once you have created the definitions you need to take extreme caution when changing any part of -the namespace. If you change the name of a `StateDefinition` pointing at `"disk"` without also -migrating data from the old name to the new name you will lose data. Data pointing at `"memory"` can -have its name changed. +Once you have created the definitions you need to take extreme caution when changing any part of the +namespace. If you change the name of a `StateDefinition` pointing at `"disk"` without also migrating +data from the old name to the new name you will lose data. Data pointing at `"memory"` can have its +name changed. ::: #### `StateDefinition` +:::note + +Secure storage is not currently supported as a storage location in the State Provider Framework. For +now, don't migrate data that is stored in secure storage but please contact the Platform team when +you have data you wanted to migrate so we can prioritize a long-term solution. If you need new data +in secure storage, use `StateService` for now. + +::: + `StateDefinition` is a simple API but a very core part of making the State Provider Framework work smoothly. It defines a storage location and top-level namespace for storage. Teams will interact with it only in a single `state-definitions.ts` file in the -[`clients`](https://github.com/bitwarden/clients) repository. This file is located under Platform team -code ownership but teams are expected to create edits to it. A team will edit this file to include a -line such as: +[`clients`](https://github.com/bitwarden/clients) repository. This file is located under Platform +team code ownership but teams are expected to create edits to it. A team will edit this file to +include a line such as: ```typescript export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk"); @@ -47,23 +56,28 @@ export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk"); The first argument to the `StateDefinition` constructor is expected to be a human readable, camelCase-formatted name for your domain or state area. The second argument will either be the string literal `"disk"` or `"memory"` dictating where all the state using this `StateDefinition` -should be stored. The Platform team will be responsible to reviewing all new and updated entries in -this file and will be looking to make sure that there are no duplicate entries containing the same -state name and state location. Teams _can_ have the same state name used for both `"disk"` and -`"memory"` locations. Tests are included to ensure this uniqueness and core naming guidelines so you -can ensure a review for a new `StateDefinition` entry can be done promptly and with very few -surprises. +should be stored. + +The Platform team will be responsible to reviewing all new and updated entries in this file and will +be looking to make sure that there are no duplicate entries containing the same state name and state +location. Teams _can_ have the same state name used for both `"disk"` and `"memory"` locations. +Tests are included to ensure this uniqueness and core naming guidelines so you can ensure a review +for a new `StateDefinition` entry can be done promptly and with very few surprises. _TODO: Make tests_ -:::note +##### Client-specific storage locations -Secure storage is not currently supported as a storage location in the State Provider Framework. For -now, don't migrate data that is stored in secure storage but please contact the Platform team when you have -data you wanted to migrate so we can prioritize a long-term solution. If you need new data in secure -storage, use `StateService` for now. +An optional third parameter to the `StateDefinition` constructor is provided if you need to specify +client-specific storage location for your state. -::: +This will most commonly be used to handle the distinction between session and local storage on the +web client. The default `"disk"` storage for the web client is session storage, and local storage +can be specified by defining your state as: + +```typescript +export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk", { web: "disk-local" }); +``` #### `KeyDefinition` @@ -383,6 +397,35 @@ memory. This can mean you might be able to drop the `*Data` class pattern for yo `*Data` class generally represented the JSON safe version of your state which we now do automatically through the `Jsonify` given to your in your `deserializer` method. +### How do `StateService` storage options map to `StateDefinition`s? + +When moving state from `StateService` to the state provider pattern, you'll be asked to create a +`StateDefinition` for your state. This should be informed by the storage location that was being +used in the `StateService`. You can use the cross-reference below to help you decide how to map +between the two. + +| `StateService` Option | Desired Storage Location | Desired Web Storage Location | `StateDefinition` Equivalent | +| ------------------------------- | ------------------------ | ---------------------------- | ------------------------------------------------------------- | +| `defaultOnDiskOptions()` | Disk | Session | `new StateDefinition("state", "disk")` | +| `defaultOnDiskLocalOptions()` | Disk | Local | `new StateDefinition("state", "disk", { web: "disk-local" })` | +| `defaultOnDiskMemoryOptions()` | Disk | Session | `new StateDefinition("state", "disk")` | +| `defaultInMemoryOptions()` | Memory | Memory | `new StateDefinition("state", "memory")` | +| `defaultSecureStorageOptions()` | Disk | N/A | No migration path currently | + +#### Clarifying `defaultOnDiskMemoryOptions()` + +Despite its name, `defaultOnDiskMemoryOptions()` results in the web client storing the state in +session storage, _not_ in memory. As such, the equivalent `StateDefinition` storage location is +`"disk"`; since `"disk"` maps to session storage on the web client there is no reason to specify +`{ web: "memory" }` as a client-specific storage location if your previous state service options +used `defaultOnDiskMemoryOptions()`. + +However, we do have cases in which the `StateService` is extended in a particular client and +different storage options are defined there for a given element of state. For example, +`defaultOnDiskMemoryOptions()` is defined on the base `StateService` but `defaultInMemoryOptions()` +is defined on the web implementation. To replicate this behavior with a `StateDefinition` you would +use `new StateDefinition("state", "disk", { web: "memory" })`. + ## Structure ![State Diagram](State_Diagram.svg) From 283c33db3e459207fed1d90e9076f83353583091 Mon Sep 17 00:00:00 2001 From: Todd Martin Date: Sat, 23 Mar 2024 14:05:34 -0400 Subject: [PATCH 16/20] Updated to include clearOn and UserKeyDefinition --- docs/architecture/deep-dives/state/index.md | 76 +++++++++++++-------- 1 file changed, 46 insertions(+), 30 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index feb4329e..587eba3e 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -79,57 +79,73 @@ can be specified by defining your state as: export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk", { web: "disk-local" }); ``` -#### `KeyDefinition` +#### `KeyDefinition` and `UserKeyDefinition` -`KeyDefinition` builds on the idea of [`StateDefinition`](#statedefinition) but it gets more -specific about the data to be stored. `KeyDefinition`s can also be instantiated in your own team's -code. This might mean creating it in the same file as the service you plan to consume it or you may -want to have a single `key-definitions.ts` file that contains all the entries for your team. Some -example instantiations are: +`KeyDefinition` and `UserKeyDefinition` build on the [`StateDefinition`](#statedefinition), +specifying a single element of state data within the `StateDefinition`. + +The framework provides both `KeyDefinition` and `UserKeyDefinition` for teams to use. The +`UserKeyDefinition` should be used for defining pieces of state that are scoped at a user level. +These will be consumed via the [`ActiveUserState`](#activeuserstatet) or +[`SingleUserState`](#singleuserstatet) within your consuming services and components. The +`UserKeyDefinition` extends the `KeyDefinition` and provides a way to specify how the state will be +cleaned up on specific user account actions. + +`KeyDefinition`s and `UserKeyDefinition`s can also be instantiated in your own team's code. This +might mean creating it in the same file as the service you plan to consume it or you may want to +have a single `key-definitions.ts` file that contains all the entries for your team. Some example +instantiations are: ```typescript -const MY_DOMAIN_DATA = new KeyDefinition(MY_DOMAIN_DISK, "data", { +const MY_DOMAIN_DATA = new UserKeyDefinition(MY_DOMAIN_DISK, "data", { // convert to your data from serialized representation `{ foo: string }` to fully-typed `MyState` deserializer: (jsonData) => MyState.fromJSON(jsonData), + clearOn: ["logout"], // can be lock, logout, both, or an empty array }); // Or if your state is an array, use the built-in helper -const MY_DOMAIN_DATA: KeyDefinition = KeyDefinition.array( +const MY_DOMAIN_DATA: UserKeyDefinition = UserKeyDefinition.array( MY_DOMAIN_DISK, "data", { deserializer: (jsonDataElement) => MyState.fromJSON(jsonDataElement), // provide a deserializer just for the element of the array }, + { + clearOn: ["logout"], + }, ); // record -const MY_DOMAIN_DATA: KeyDefinition> = +const MY_DOMAIN_DATA: UserKeyDefinition> = KeyDefinition.record(MY_DOMAIN_DISK, "data", { deserializer: (jsonDataValue) => MyState.fromJSON(jsonDataValue), // provide a deserializer just for the value in each key-value pair + clearOn: ["logout"], }); ``` -The first argument to `KeyDefinition` is always the `StateDefinition` that this key should belong -to. The second argument should be a human readable, camelCase-formatted name of the `KeyDefinition`. -For example, the accounts service may wish to store a known accounts array on disk and choose -`knownAccounts` to be the second argument. This name should be unique amongst all other -`KeyDefinition`s that consume the same `StateDefinition`. The responsibility of this uniqueness is -on the team. As such, you should never consume the `StateDefinition` of another team in your own -`KeyDefinition`. The third argument is an object of type -[`KeyDefinitionOptions`](#keydefinitionoptions). - -##### `KeyDefinitionOptions` - -`deserializer` (required) - Takes a method that gives you your state in it's JSON format and makes -you responsible for converting that into JSON back into a full JavaScript object, if you choose to -use a class to represent your state that means having its prototype and any method you declare on -it. If your state is a simple value like `string`, `boolean`, `number`, or arrays of those values, -your deserializer can be as simple as `data => data`. But, if your data has something like `Date`, -which gets serialized as a string you will need to convert that back into a `Date` like: -`data => new Date(data)`. - -`cleanupDelayMs` (optional) - Takes a number of milliseconds to wait before cleaning up the state -after the last subscriber has unsubscribed. Defaults to 1000ms. +The arguments for defining a `KeyDefinition` or `UserKeyDefinition` are: + +| Argument | Usage | +| ----------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `stateDefinition` | The `StateDefinition` to which that this key belongs | +| `key` | A human readable, camelCase-formatted name for the key definition. This name should be unique amongst all other `KeyDefinition`s or `UserKeyDefinition`s that consume the same `StateDefinition`. | +| `options` | An object of type [`KeyDefinitionOptions`](#key-definition-options) or [`UserKeyDefinitionOptions`](#key-definition-options), which defines the behavior of the key. | + +:::warning + +It is the responsibility of the team to ensure the uniqueness of the `key` within a +`StateDefinition`. As such, you should never consume the `StateDefinition` of another team in your +own key definition. + +::: + +##### Key Definition Options + +| Option | Required? | Usage | +| ---------------- | ---------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `deserializer` | Yes | Takes a method that gives you your state in it's JSON format and makes you responsible for converting that into JSON back into a full JavaScript object, if you choose to use a class to represent your state that means having its prototype and any method you declare on it. If your state is a simple value like `string`, `boolean`, `number`, or arrays of those values, your deserializer can be as simple as `data => data`. But, if your data has something like `Date`, which gets serialized as a string you will need to convert that back into a `Date` like: `data => new Date(data)`. | +| `cleanupDelayMs` | No | Takes a number of milliseconds to wait before cleaning up the state after the last subscriber has unsubscribed. Defaults to 1000ms. | +| `clearOn` | Yes, for `UserKeyDefinition` | An additional parameter provided for `UserKeyDefinition` **only**, which allows specification of the user account `ClearEvent`s that will remove the piece of state from persistence. The available values for `ClearEvent` are `logout`, `lock`, or both. An empty array should be used if the state should not ever be removed (e.g. for settings). | ### `StateProvider` From fcdc588a57e8712395ee87cf20083abe3a606ea2 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Thu, 19 Sep 2024 13:55:00 -0400 Subject: [PATCH 17/20] Update `ActiveUserState` Information --- docs/architecture/deep-dives/state/index.md | 106 ++++++++++++++++---- 1 file changed, 84 insertions(+), 22 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 84e5d9bd..f3dc0b0c 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -8,7 +8,7 @@ trustworthy observable stream of that state. - [Storage definitions](#storage-definitions) - [`StateDefinition`](#statedefinition) - - [`KeyDefinition`](#keydefinition) + - [`KeyDefinition` & `UserKeyDefinition`](#keydefinition-and-userkeydefinition) - [`StateProvider`](#stateprovider) - [`ActiveUserState`](#activeuserstatet) - [`GlobalState`](#globalstatet) @@ -64,8 +64,6 @@ location. Teams _can_ have the same state name used for both `"disk"` and `"memo Tests are included to ensure this uniqueness and core naming guidelines so you can ensure a review for a new `StateDefinition` entry can be done promptly and with very few surprises. -_TODO: Make tests_ - ##### Client-specific storage locations An optional third parameter to the `StateDefinition` constructor is provided if you need to specify @@ -202,6 +200,13 @@ class FolderService { ### `ActiveUserState` +:::warning + +`ActiveUserState` has problems with consider not using it anymore, +[read more](#should-i-use-activeuserstate). + +::: + `ActiveUserState` is an object to help you maintain and view the state of the currently active user. If the currently active user changes, like through account switching, the data this object represents will change along with it. Gone is the need to subscribe to @@ -211,34 +216,16 @@ represents will change along with it. Gone is the need to subscribe to ```typescript interface ActiveUserState { state$: Observable; - update: (updateState: (state: T, dependency: TCombine) => T, stateUpdateOptions?: StateUpdateOptions): Promise; } ``` -:::note - -Specifics around `StateUpdateOptions` are discussed in the [Advanced Usage](#advanced-usage) -section. - -::: - -The `update` method takes a function `updateState: (state: T) => T` that can be used to update the -state in both a destructive and additive way. The function gives you a representation of what is -currently saved as your state and it requires you to return the state that you want saved into -storage. This means if you have an array on your state, you can `push` onto the array and return the -array back. The return value of the `updateState` function is always used as the new state value -- -do not rely on object mutation to update! - The `state$` property provides you with an `Observable` that can be subscribed to. `ActiveUserState.state$` will emit for the following reasons: - The active user changes. - The chosen storage location emits an update to the key defined by `KeyDefinition`. This can occur for any reason including: - - You caused an update through the `update` method. - - Another service in a different context calls `update` on their own instance of - `ActiveUserState` made from the same `KeyDefinition`. - - A `SingleUserState` method pointing at the same `KeyDefinition` as `ActiveUserState` and + - A `SingleUserState` method pointing at the same `UserKeyDefinition` as `ActiveUserState` and pointing at the user that is active that had `update` called - Someone updates the key directly on the underlying storage service _(please don't do this)_ @@ -443,6 +430,81 @@ different storage options are defined there for a given element of state. For ex is defined on the web implementation. To replicate this behavior with a `StateDefinition` you would use `new StateDefinition("state", "disk", { web: "memory" })`. +### Should I use `ActiveUserState`? + +Probably not, `ActiveUserState` is either currently in the process of or already completed the +removal of its `update` method. This will effectively make it readonly, but you should consider +maybe not even using it for reading either. `update` is actively bad, while reading is just not as +dynamic of a API design. + +Take the following example: + +```typescript +private folderState: ActiveUserState> + +renameFolder(folderId: string, newName: string) { + // Get state + const folders = await firstValueFrom(this.folderState.state$); + // Mutate state + folders[folderId].name = await encryptString(newName); + // Save state + await this.folderState.update(() => folders); +} +``` + +You can imagine a scenario where the active user changes between the read and the write. This would +be a big problem because now user A's folders was stored in state for user B. By taking a user id +and utilizing `SingleUserState` instead you can avoid this problem by passing ensuring both +operation happen for the same user. This is obviously an extreme example where the point between the +read and write is pretty minimal but there are places in our application where the time between is +much larger. Maybe information is read out and placed into a form for editing and then the form can +be submitted to be saved. + +The first reason for why you maybe shouldn't use `ActiveUserState` for reading is for API +flexibility. Even though you may not need an API to return the data of a non-active user right now, +you or someone else may want to. If you have a method that takes the `UserId` then it can be +consumed by someone passing in the active user or by passing a non-active user. You can now have a +single API that is useful in multiple scenarios. + +The other reason is so that you can more cleanly switch users to new data when multiple streams are +in play. Consider the following example: + +```typescript +const view$ = combineLatest([ + this.folderService.activeUserFolders$, + this.cipherService.activeUserCiphers$, +]).pipe(map(([folders, ciphers]) => buildView(folders, ciphers))); +``` + +Since both are tied to the active user, you will get one emission when first subscribed to and +during an account switch, you will likely get TWO other emissions. One for each, inner observable +reacting to the new user. This could mean you try to combine the folders and ciphers of two +accounts. This is ideally not a huge issue because the last emission will have the same users data +but it's not ideal, and easily avoidable. Instead you can write it like this: + +```typescript +const view$ = this.accountService.activeAccount$.pipe( + switchMap((account) => { + if (account == null) { + throw new Error("This view should only be viewable while there is an active user."); + } + + return combineLatest([ + this.folderService.userFolders$(account.id), + this.cipherService.userCiphers$(account.id), + ]); + }), + map(([folders, ciphers]) => buildView(folders, ciphers)), +); +``` + +You have to write a little more code but you do a few things that might force you to think about the +UX and rules around when this information should be viewed. With `ActiveUserState` it will simply +not emit while there is no active user. But with this, you can choose what to do when there isn't an +active user and you could simple add a `first()` to the `activeAccount$` pipe if you do NOT want to +support account switching. An account switch will also emit the `combineLatest` information a single +time and the info will be always for the same account. + ## Structure ![State Diagram](State_Diagram.svg) From 0c00d9a6e21623bbdbe4f4c2002c6a24315856f2 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 23 Sep 2024 09:18:31 -0400 Subject: [PATCH 18/20] Fix AUS Warning MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: ✨ Audrey ✨ --- docs/architecture/deep-dives/state/index.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index f3dc0b0c..5a35f03d 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -202,8 +202,8 @@ class FolderService { :::warning -`ActiveUserState` has problems with consider not using it anymore, -[read more](#should-i-use-activeuserstate). +`ActiveUserState` has race condition problems. Do not use it for updates and consider transitioning +your code to SingleUserState instead. [Read more](#should-i-use-activeuserstate) ::: From 8984c94cf14830aefc13a987123856f1932eaf4b Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 23 Sep 2024 09:32:40 -0400 Subject: [PATCH 19/20] Change to instruction MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: ✨ Audrey ✨ --- docs/architecture/deep-dives/state/index.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 5a35f03d..09cccff2 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -82,9 +82,9 @@ export const MY_DOMAIN_DISK = new StateDefinition("myDomain", "disk", { web: "di `KeyDefinition` and `UserKeyDefinition` build on the [`StateDefinition`](#statedefinition), specifying a single element of state data within the `StateDefinition`. -The framework provides both `KeyDefinition` and `UserKeyDefinition` for teams to use. The -`UserKeyDefinition` should be used for defining pieces of state that are scoped at a user level. -These will be consumed via the [`ActiveUserState`](#activeuserstatet) or +The framework provides both `KeyDefinition` and `UserKeyDefinition` for teams to use. Use +`UserKeyDefinition` for state scoped to a user and `KeyDefinition` for user-independent state. These +will be consumed via the [`ActiveUserState`](#activeuserstatet) or [`SingleUserState`](#singleuserstatet) within your consuming services and components. The `UserKeyDefinition` extends the `KeyDefinition` and provides a way to specify how the state will be cleaned up on specific user account actions. From 6e57fbc1b75259f6c2d7ecfa80f44afb1208c2c8 Mon Sep 17 00:00:00 2001 From: Justin Baur <19896123+justindbaur@users.noreply.github.com> Date: Mon, 23 Sep 2024 12:57:12 -0400 Subject: [PATCH 20/20] Move to Present Tense --- docs/architecture/deep-dives/state/index.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/architecture/deep-dives/state/index.md b/docs/architecture/deep-dives/state/index.md index 09cccff2..bbd0fbf4 100644 --- a/docs/architecture/deep-dives/state/index.md +++ b/docs/architecture/deep-dives/state/index.md @@ -58,11 +58,11 @@ camelCase-formatted name for your domain or state area. The second argument will string literal `"disk"` or `"memory"` dictating where all the state using this `StateDefinition` should be stored. -The Platform team will be responsible to reviewing all new and updated entries in this file and will -be looking to make sure that there are no duplicate entries containing the same state name and state -location. Teams _can_ have the same state name used for both `"disk"` and `"memory"` locations. -Tests are included to ensure this uniqueness and core naming guidelines so you can ensure a review -for a new `StateDefinition` entry can be done promptly and with very few surprises. +The Platform team is responsible for reviewing all new and updated entries in this file and makes +sure that there are no duplicate entries containing the same state name and state location. Teams +are able to have the same state name used for both `"disk"` and `"memory"` locations. Tests are +included to ensure this uniqueness and core naming guidelines so teams can ensure a review for a new +`StateDefinition` entry is done promptly and with very few surprises. ##### Client-specific storage locations