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

PersistenceProvider clearing of default state should be an explicit option on target #3945

Open
amcclain opened this issue Mar 14, 2025 · 0 comments

Comments

@amcclain
Copy link
Member

amcclain commented Mar 14, 2025

Within PersistenceProvider.bindToTarget, we added a reaction that will actively clear the persisted state if it comes to equal the default state:

this.disposer = reaction(

The intention of this was to avoid eagerly capturing or "snapshotting" default state in such a way that code changes establishing a new default would not be respected. We were very focused on the case of grid columns, especially with preferences, which are "auto saving" by nature.

I.e. we wanted to avoid the following, which we saw as an anti-pattern:

  • Grid persisted to pref
  • User visits grid, changes nothing about the columns, has no opinion on the columns, but snapshot of default state is saved to pref
  • App code is changed to introduce a new column that is visible by default
  • User is told about new column, looks for it in grid, but does not see it.

That change was made with the big updates to persistence several months ago.

Now, working on a client app with a custom viewManager-ish implementation, we observed the following:

  • The custom vm impl had a "new view" option that we did not want to remove. We went to implement this by creating a combined default state object for all persisted components - we asked each component's provider for its default state and installed it at the right path. Our expectation was that this would correctly represent the default state of the components.
  • We saved that state in our persisted view record and then pushed it onto the components
  • The components naturally round-tripped the state back through the providers.
  • We expected our view to remain clean - ie the state we pushed onto components was their default state, so the state they then report should match.
  • For most components that was the case, but for a grid, the columns came back from the round-trip erased. So our newly saved view was immediately dirty in a surprising way.

We tracked this down to the erase-default-state behavior, but noticed it wasn't happening to all state - ie grid erased its columns, but not its default sortBy

Issue 1 with current impl:

  • It is handled by a reaction that might or might not be triggered by simply instantiating the component. Some underlying rendered state might change (eg columns autosized) that can trigger this reaction in a way that erases the persisted state, but other bits of state (eg sortBy string) won't trigger this reaction and won't cause default state to be erased. So if and when this erasing happens is a bit unpredictable.

Issue 2:

  • While the rationale above still makes sense for grid columns, I don't think it generalizes very well.
  • E.g. we have saved state that includes a boolean showActiveItemsOnly. Say the default in-code value is true.
  • User sets up a view, flips that switch off then on again, then adds a filter on a customer and saves it as "Active items for Customer X" - which is exactly what they are seeing at that time. That activeOnly setting is essential to their report, but happens to match the default and is not persisted.
  • In an unrelated decision, the default in the code is changed to false and a new version of the app is released.
  • User returns to their saved view, which now unexpectedly (and without them doing anything) shows all items, not just active ones.

Proposal:

  • Add a new optional getter to the Persistable interface - something like persistDefaultValue: true.
  • Check that flag when setting up the reaction - only do so if false.
  • Set to false for grid columns here -
  • Consider also allowing PersistenceProvider instance to have a say - ie work into PersistOptions
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

When branches are created from issues, their pull requests are automatically linked.

1 participant