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]

getUserState$ is a shorthand for returning the state$ property so you can't call update on it.

Suggested change

	await this.stateProvider.getUserState$(userId, DOMAIN_USER_STATE).update((state) => null);
	await this.stateProvider.getUser(userId, DOMAIN_USER_STATE).update((state) => null);

[trmartin4]

Fixed in fbd3861 (#549). Thank you for catching that.

[justindbaur]

Missing end to the warning block?

[trmartin4]

Oops! Fixed in fbd3861 (#549).

[justindbaur]

Somewhere in here we can also mention that update will return the new value that was just saved back to the caller, or in the case of a custom shouldUpdate function that returned false it will return the existing state value. So if consumers need the updated value they can always get it that way.

[trmartin4]

Addressed with fbd3861 (#549). Let me know if you think the warning block is too big and should just be a new section.