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

Global State Rewrite #7515

Merged
merged 3 commits into from
Jan 11, 2024
Merged

Global State Rewrite #7515

merged 3 commits into from
Jan 11, 2024

Conversation

justindbaur
Copy link
Member

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

We rewrote the inner workings of DefaultActiveUserState and DefaultSingleUserState to rely more on various rxjs offerings instead of rolling our own. This migrates DefaultGlobalState to do the same.

Also, during migrating it we made a decision to not do an inner subscription to state$ and instead always go and get the data from state ourselves. This was applied to the other states.

Code changes

  • libs/common/src/platform/state/implementations/default-active-user-state.spec.ts: Update expectation of test to show that we do expect a call to update to always go and get state.
  • libs/common/src/platform/state/implementations/default-active-user-state.ts: Update it to always go to storage on update.
  • libs/common/src/platform/state/implementations/default-global-state.spec.ts: Rework tests to show cold and hot in the new way.
  • libs/common/src/platform/state/implementations/default-global-state.ts: Rely more on share.
  • libs/common/src/platform/state/implementations/default-single-user-state.ts: Update single user state to always go to storage service for update state.

Screenshots

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@justindbaur justindbaur requested a review from a team as a code owner January 11, 2024 13:37
@github-actions github-actions bot added the needs-qa Marks a PR as requiring QA approval label Jan 11, 2024
Copy link

codecov bot commented Jan 11, 2024

Codecov Report

Attention: 3 lines in your changes are missing coverage. Please review.

Comparison is base (1f57244) 63.03% compared to head (b6d609d) 62.92%.
Report is 5 commits behind head on main.

Files Patch % Lines
...form/state/implementations/default-global-state.ts 83.33% 2 Missing ⚠️
...state/implementations/default-active-user-state.ts 85.71% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7515      +/-   ##
==========================================
- Coverage   63.03%   62.92%   -0.12%     
==========================================
  Files         756      756              
  Lines       21492    21451      -41     
  Branches     4270     4262       -8     
==========================================
- Hits        13548    13498      -50     
- Misses       7130     7136       +6     
- Partials      814      817       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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 not sure about that null check, presumably you had a test that suggested it.

Can you validate this stronger version?

@justindbaur justindbaur requested a review from MGibson1 January 11, 2024 17:56
@MGibson1 MGibson1 removed the needs-qa Marks a PR as requiring QA approval label Jan 11, 2024
@justindbaur justindbaur merged commit 0874df8 into main Jan 11, 2024
62 of 64 checks passed
@justindbaur justindbaur deleted the global-state-refactor branch January 11, 2024 21:32
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.

2 participants