Updates to state provider deep dive

[trmartin4]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-14197
https://bitwarden.atlassian.net/browse/PM-19220
https://bitwarden.atlassian.net/browse/PM-19218
https://bitwarden.atlassian.net/browse/PM-19219

📔 Objective

• Moved the section on update out of the "Advanced Usage" section, since we are going to be encouraging teams to use it to avoid unnecessary I/O for state updates (see Reduce desktop disk writes clients#13271 for an example). Before we start asking teams to pay more attention to this, I wanted to make sure it had a good place in the docs.
• Re-ordered the definitions to move ActiveUserState to the bottom, since it is no longer recommended.
• Removed some of the references to migrations, since they are mostly complete.
• Documented cleanupDelayMs option behavior when it's set to 0.
• Documented that we discourage firstValueFrom(state$) directly after update.
• Documented that state$ does not guarantee an emission on same value updates.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
  issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	fbd3861
Status:	✅  Deploy successful!
Preview URL:	https://157b4563.contributing-docs.pages.dev
Branch Preview URL:	https://state-provider-updates.contributing-docs.pages.dev

View logs

[github-actions]

Checkmarx One – Scan Summary & Details – a1ee5c2c-3c93-4695-a5f8-952eb80f3bfe

Great job, no security vulnerabilities found in this Pull Request

[trmartin4]

❓ @justindbaur Do you think that it's OK to remove the section on Migration and the details on how StateService definitions map to StateProvider definitions? I don't want to clutter up the docs with things that teams aren't using today, but I also see the value in having it for reference in the future. Perhaps a sub-page? Or are you OK with keeping it in git history? Or would you prefer to leave it as-is?

[justindbaur]

@trmartin4 Thank you for this, I had thought about adding some more docs regarding update now that we are going to recommend it more.

I do think having a brief blurb about state migrations is good as teams will still occasionally have to do them. We don't need to keep around the state service to state provider mappings though. Since state migrations shouldn't change after being written though it could be useful to just defer to here for any examples and we don't need one here in the docs.

As for shouldUpdate I think it might be time for a new example(s). What we are about to ask people is to do more than just avoid saving null when it's already null and instead asking them to avoid saving the same value over and over. So I was thinking about adding something like the following:

=============

A simple example using shouldUpdate on simple JavaScript primitive types that work nicely with the strict equal operator (===).

const USES_KEYCONNECTOR: UserKeyDefinition<boolean> = ...;

async setUsesKeyConnector(value: boolean, userId: UserId) {
  // Only do the update if the current value saved in state
  // differs in equality of the incoming value.
  await this.stateProvider.getUser(userId, USES_KEYCONNECTOR).update(
    currentValue => currentValue !== value
  );
}

Implementing a custom equality operator

type Cipher = { id: string, username: string, password: string, revisionDate: Date };
const LAST_USED_CIPHER: UserKeyDefinition<Cipher> = ...;

async setLastUsedCipher(lastUsedCipher: Cipher | null, userId: UserId) {
  await this.stateProvider.getUser(userId, LAST_USED_CIPHER).update(
    currentValue => !this.areEqual(currentValue, lastUsedCipher)
  );
}

areEqual(a: Cipher | null, b: Cipher | null) {
  if (a == null) {
    return b == null;
  }

  if (b == null) {
    return false;
  }

  // Option one - Full equality, comparing every property for value equality
  return a.id === b.id && 
    a.username === b.username && 
    a.password === b.password &&
    a.revisionDate === b.revisionDate;

  // Option two - Partial equality based on requirement that any update would
  // bump the revision date.
  return a.id === b.id && a.revisionDate === b.revisionDate;
}

It's important that if you implement an equality function that you then negate the output of that function for use in shouldUpdate since you will want to go through the update when they are NOT the same value.

=============

[trmartin4]

@justindbaur I addressed your concerns, if you don't mind taking a look.

[trmartin4]

@justindbaur added today's feedback 👍

[justindbaur]

Just a few comments, thank you so much!

[justindbaur]

Missing end to the warning block?

[trmartin4]

Oops! Fixed in fbd3861 (#549).

[justindbaur]

Thank you so much

[MGibson1]

A few tweaks, but the biggest one is an omission from the original -- async updates.

[MGibson1]

Why remove active user state from here, but not the

#### `KeyDefinition` and `UserKeyDefinition`

section?

It's also referenced below. Let's acknowledge its existence here, but introduce it as a deprecated method that attempted to resolve the state of the active user, but produces race conditions.

[MGibson1]

observation, I don't really think this needs to change here, but this isn't yet deprecated in the interface.

[MGibson1]

issue Let's maintain the T | null type parameters for state. This would be true of roughtly every type parameter in this file.

[MGibson1]

issue This section should mention combinedState$ somewhere

[MGibson1]

Suggested change

	`ActiveUserState` has race condition problems. Do not use it for updates and consider transitioning
	`ActiveUserState` has race condition problems. Do not add usages and consider transitioning

[MGibson1]

duplicative

Suggested change

	If you do need to obtain the result of an update in a non-reactive way, you should use the result
	returned from the `update()` method. This should be used instead of immediately re-requesting the
	value through `firstValueFrom()`. The `update()` will return the value that will be persisted to
	state, after any `shouldUpdate()` filters are applied.
	If you do need to obtain the result of an update in a non-reactive way, you should use the result
	returned from the `update()` method. The `update()` will return the value that will be persisted to
	state, after any `shouldUpdate()` filters are applied.

[MGibson1]

Suggested change

	- We don't guarantee that your `updateState` function is called the instant that the `update` method
	- We don't guarantee that your `updateState` callback is called the instant that the `update` method

[MGibson1]

issue This section is missing a large use case for async updates and rx transform pipelines. combineLatestWith can be used to handle updates where the new value depends on either async code or you'd prefer to handle generation of a new value in an observable transform flow.

    const state = this.stateProvider.get(userId, SavedFiltersStateDefinition);

    const transform: OperatorFunction<any, any> = pipe(
      // perform some transforms
      map((value) => value),
    )

    async function transformAsync<T>(value: T) {
      return Promise.resolve(value);
    }

    await state.update((_, newState) => newState, {
      // Actual processing to generate the new state is done here
      combineLatestWith: state.state$.pipe(
        mergeMap(async (old) => {
          return await transformAsync(old);
        }),
        transform,
      ),
      shouldUpdate: (oldState, newState) => !areEqual(oldState, newState),
    })

[MGibson1]

Do we have a doc to link here? If not, please link to a github permalink until we can create one.