Updates to state provider deep dive

[trmartin4]

trafficstars

🎟️ 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 https://github.com/bitwarden/clients/pull/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[bot]]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	daf03ce
Status:	✅  Deploy successful!
Preview URL:	https://00f0eb5b.contributing-docs.pages.dev
Branch Preview URL:	https://state-provider-updates.contributing-docs.pages.dev

View logs

[github-actions[bot]]

Checkmarx One – Scan Summary & Details – d7992318-c01f-466c-9ae5-97f39b8ff9c6

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 👍

[trmartin4]

@MGibson1 I have addressed all of your concerns, with the exception of https://github.com/bitwarden/contributing-docs/pull/549#discussion_r2005924611.

I am not sure that we'll actually be merging this PR though, as we may instead move the Deep Dive out of contributing docs and into the markdown documentation for our shiny new state Nx package on clients that we're working on carving out. However, I want to make sure all your concerns are addressed so that I can port over the markdown and be confident that it will be accurate.

[trmartin4]

Closing this pull request, as we would like to incorporate the state provider into the markdown documentation for the new @bitwarden/state package in clients, introduced in https://github.com/bitwarden/clients/pull/15772.