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.

Sign up for GitHub

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

Jump to bottom

Persistent stores should merge persisted states into default states by default #6233

Open
dbjorge opened this issue Dec 2, 2022 · 2 comments
Open

Persistent stores should merge persisted states into default states by default #6233

dbjorge opened this issue Dec 2, 2022 · 2 comments
Labels
category: engineering task

Comments

@dbjorge
Copy link
Contributor

dbjorge commented Dec 2, 2022

Describe the bug

This is a tracking issue for a bit of engineering followup to #6231 that @madalynrose, @waabid, and I agree that we want to defer til after our initial MV3 release (due to being somewhat risky) but implement soon afterwards (for correctness in future upgrade scenarios).

#6231 updates the VisualizationStore initialization path to produce initial store state by merging any persisted state on top of the store's default state, rather than using the persistent-store's current default behavior of keeping persisted state and ignoring default state when persisted state is available. This is necessary to handle an upgrade scenario where a tab might have persisted store state that doesn't include the new tests.mediumPass property.

This issue happened to hit VisualizationStore today because it's the store that we happened to update recently, but the underlying issue isn't specific to any one store; this could occur for any store that receives new properties that are expected to be always-initialized in a default state.

We think the correct fix is to update PersistentStore's default initial state calculation behavior to handle "there is persisted state available" by merging persisted state into default state, instead of taking persisted state as-is like it does today. This should make it feasible to simply VisualizationStore and revert most of the new behavior added in #6231. It might also enable a similar simplification for other cases that use custom getDefaultState behavior (I think only AssessmentStore currently).
@ghost ghost added the status: new This issue is new and requires triage by DRI. label Dec 2, 2022
@dbjorge dbjorge added the status: blocked This issue is blocked. label Dec 2, 2022
@dbjorge
Copy link
Contributor Author

dbjorge commented Dec 2, 2022

Marking as blocked until the initial MV3 release has happened and had a little bake time.

@dbjorge dbjorge removed the status: new This issue is new and requires triage by DRI. label Dec 2, 2022
@dbjorge dbjorge changed the title Persistent stores should merge default states into persisted states by default Persistent stores should merge persisted states into default states by default Dec 2, 2022
@dbjorge dbjorge mentioned this issue Dec 2, 2022
Merged
5 tasks
@dbjorge dbjorge added status: active This issue is currently being worked on by someone. and removed status: blocked This issue is blocked. labels Dec 28, 2022
@dbjorge dbjorge mentioned this issue Dec 28, 2022
Closed
4 tasks
@DaveTryon DaveTryon added status: resolved This issue has been merged into main and deployed to canary. status: active This issue is currently being worked on by someone. and removed status: active This issue is currently being worked on by someone. status: resolved This issue has been merged into main and deployed to canary. labels May 9, 2023
@DaveTryon
Copy link
Contributor

According to Dan, this hasn't yet been fixed

@DaveTryon DaveTryon removed the status: active This issue is currently being worked on by someone. label May 10, 2023
@codeofdusk codeofdusk mentioned this issue Aug 4, 2023
Merged
3 tasks
codeofdusk added a commit that referenced this issue Aug 4, 2023
@codeofdusk @dbjorge
refactor: remove obsolete persistStoreData flag from PersistentStore,…
a284afe 
… take 2 (#6864)

When we originally introduced `PersistentStore` during the MV2->MV3
transition, it included a `persistStoreData` constructor flag which
allowed us to gradually transition different stores to using persistence
depending on whether we were working with an MV2 or MV3 build. This
functionality is now obsolete - all `PersistentStore`-based stores
always use this with a constant `true` input. This commit removes the
obsolete flag.

This is a takeover of abandoned #6300, which was closed due to the
flag's usage in Unified. Now that Unified is gone, this change is
unblocked.

Noticed while working on #6233

Co-authored-by: Dan Bjorge <[email protected]>
@codeofdusk codeofdusk self-assigned this Aug 4, 2023
@codeofdusk codeofdusk added the active (Dev) The assigned dev is working to perform the work. label Aug 4, 2023
@codeofdusk codeofdusk mentioned this issue Aug 16, 2023
Merged
4 tasks
codeofdusk added a commit that referenced this issue Aug 16, 2023
@codeofdusk
fix: Merge persisted state into default state by default (#6898)
2922ea0 
This commit updates `PersistantStore.generateDefaultState` to merge in
`persistedState` to `defaultState`, rather than just returning
`persistedState` as-is.

Addresses #6233.
@DaveTryon DaveTryon removed the active (Dev) The assigned dev is working to perform the work. label Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
category: engineering task
Projects
Accessibility Insights
Status: Tabled
Milestone
No milestone
Development

No branches or pull requests
3 participants
@dbjorge @codeofdusk @DaveTryon