Added configurability for PersistenceProvider clearing of default state#4121
Added configurability for PersistenceProvider clearing of default state#4121
Conversation
| PersistenceProvider.create({ | ||
| persistOptions: { | ||
| path: `${path}.columns`, | ||
| persistDefaultValue: true, |
There was a problem hiding this comment.
Implementation of Anslem's proposal:
Set to false for grid columns here -
Note that this can be easily overriden by users using the PersistOption for persistColumns:
persistColumns: { persistDefaultValue: false }
| export interface Persistable<S> { | ||
| getPersistableState(): PersistableState<S>; | ||
| setPersistableState(state: PersistableState<S>): void; | ||
| defaultPersistableState?: PersistableState<S>; |
There was a problem hiding this comment.
This gives the user full control of the defaultPersistableState without the need to create an exotic getPersistableState() method.
There was a problem hiding this comment.
the ticket did not mention this. Is this strictly neccessary?
| /** | ||
| * If true, the default state will be persisted just like any other state, by writing its value. | ||
| * Otherwise, the default state is represented by clearing/emptying the persisted value. | ||
| */ |
There was a problem hiding this comment.
I tried hard to make this comment very clear - let me know if it can be improved.
lbwexler
left a comment
lbwexler
left a comment
There was a problem hiding this comment.
The flag to short-circuit the erasure looks pretty straightforward and safe and I approve -- the more simple hooks we have like this, the better.
Not sure I understand how the defaultPersistableState would be used? Would we change our implementations of getPersistableState() in hoist itself?
| export interface Persistable<S> { | ||
| getPersistableState(): PersistableState<S>; | ||
| setPersistableState(state: PersistableState<S>): void; | ||
| defaultPersistableState?: PersistableState<S>; |
There was a problem hiding this comment.
the ticket did not mention this. Is this strictly neccessary?
|
@jskupsik can you pls pick up on Lee's question here from last month? |
|
@jskupsik + @ghsolomon would love to get this finalized / merged if we can - Jakub, Lee has an outstanding question here, and Greg wanted to get your eyes on it as our general persistence czar. Thanks! |
3 participants
Hoist P/R Checklist
Pull request authors: Review and check off the below. Items that do not apply can also be
checked off to indicate they have been considered. If unclear if a step is relevant, please leave
unchecked and note in comments.
developbranch as of last change.breaking-changelabel + CHANGELOG if so.If your change is still a WIP, please use the "Create draft pull request" option in the split
button below to indicate it is not ready yet for a final review.