Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add State Provider Framework Deep Dive #248

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

justindbaur
Copy link
Member

Objective

To document the newly added State Provider Framework added in bitwarden/clients#6640 and refined in bitwarden/clients#6860.

@justindbaur justindbaur requested a review from a team as a code owner December 5, 2023 20:31
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
@bitwarden-bot
Copy link

bitwarden-bot commented Dec 6, 2023

Logo
Checkmarx One – Scan Summary & Detailsda201082-7755-4419-974e-b9eb4486fc75

No New Or Fixed Issues Found

Co-authored-by: Matt Gibson <[email protected]>
Copy link

cloudflare-workers-and-pages bot commented Dec 6, 2023

Deploying contributing-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: fcdc588
Status: ✅  Deploy successful!
Preview URL: https://060117a9.contributing-docs.pages.dev
Branch Preview URL: https://add-state-provider-framework.contributing-docs.pages.dev

View logs

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seagulling with just a couple things I noticed with a quick read, really for my OCD.

docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
* First pass at derived state documentation

* `npm run prettier` 🤖

* Apply suggestions from code review

Co-authored-by: Matt Bishop <[email protected]>

---------

Co-authored-by: Matt Bishop <[email protected]>
Copy link
Member

@MGibson1 MGibson1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy with this, but I'll leave it to @withinfocus since I worked on this quite a bit.

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started getting a bit wild with copyediting. There are a few still-missing sections I assume need filling in, or strike them from the PR perhaps?


## Structure

![State Diagram](State_Diagram.svg)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🌱 I'm noticing a lot of variety in our diagrams. Would be nice to standardize on look and feel.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this probably used Excalidraw, whereas we've previously used PlantUML. I personally like the slightly less formal style of this diagram, but if we'd like standardization I can move this to PlantUML if it would be helpful.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another possibility is to use a markdown-compatible renderer, such as mermaid or GoAT.

docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
docs/architecture/deep-dives/state/index.md Outdated Show resolved Hide resolved
audreyality
audreyality previously approved these changes Jan 31, 2024
#296)

* Added optional constructor parameter and updated mapping instructions.

* Fixed table

* Changed verbiage.

* Updated verbiage.
@justindbaur
Copy link
Member Author

Well it has only been 8 months but I'd love to try and merge this in 🫤.

Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not an exhaustive review because I want to see this merge!

Copy link
Contributor

@audreyality audreyality left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 I also want to see this merged. Feedback is provided, but I'm totally cool with it landing in a followup PR. :)


:::

#### `StateDefinition`

:::note

Secure storage is not currently supported as a storage location in the State Provider Framework. For
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ Is secure storage just a description, or does it refer to an emerging web api?

Comment on lines +61 to +62
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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ Consider using present tense.

Comment on lines +85 to +86
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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ Consider stating this as an instruction. "Use UserKeyDefinition for state scoped to a user and KeyDefinition for user-independent state." or similar.

[`KeyDefinitionOptions`](#keydefinitionoptions).
The arguments for defining a `KeyDefinition` or `UserKeyDefinition` are:

| Argument | Usage |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍🏻 Love presenting the options and arguments using tables!

@@ -172,6 +200,13 @@ class FolderService {

### `ActiveUserState<T>`

:::warning

`ActiveUserState` has problems with consider not using it anymore,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⛏️ Looks like you a word. "ActiveUserState has race condition problems. Do not use it for updates and consider transitioning your code to SingleUserState instead."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants