Skip to content

Store callback Output value in persistence storage #3279

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

Open
wants to merge 7 commits into
base: dev
Choose a base branch
from

Conversation

petar-qb
Copy link

Fixes #2678

This PR:

TLDR:
This PR ensures that the callback output value is written (instead of being pruned) within the browser's persistence storage for its dcc components that has persistence=True set.


For the following code (also posted in the Dash Plotly forum question that's linked above):

from dash import Dash, dcc, html, Input, Output

app = Dash(__name__)

app.layout = html.Div([
    html.Button("Select All", id="select_all_button"),
    dcc.Checklist(
        id='checkbox',
        options=[1, 2, 3],
        value=[],
        persistence=True,
        persistence_type='session'
    ),
])


@app.callback(
    Output('checkbox', 'value'),
    Input('select_all_button', 'n_clicks'),
    prevent_initial_call=True,
)
def select_all_options(n_clicks):
    return [1, 2, 3]


if __name__ == '__main__':
    app.run(debug=True)

Here's how it works from the plotly/dash::dev:

Screen.Recording.2025-01-30.at.11.07.24.mov

Here's how it works from the petar-qb/dash::feature/store-output-value-in-persistence-storage:

Screen.Recording.2025-01-30.at.11.09.25.mov

Contributor Checklist

  • I have run the tests locally and they passed. (refer to testing section in contributing)
  • I have added tests, or extended existing tests, to cover any new features or bugs fixed in this PR

optionals

  • I have added entry in the CHANGELOG.md
  • If this PR needs a follow-up in dash docs, community thread, I have mentioned the relevant URLS as follows
    • this GitHub #PR number updates the dash docs
    • here is the show and tell thread in Plotly Dash community

Copy link
Contributor

@T4rk1n T4rk1n left a comment

Choose a reason for hiding this comment

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

Looks good, but there is a failing test rdps011. I am not sure if it's because the behavior changed to persist value on callbacks or if it's a regression.

Maybe @alexcjohnson can provide more insights on this test?

Comment on lines -525 to -542
const transforms = element.persistenceTransforms || {};
for (const propName in newProps) {
const propTransforms = transforms[propName];
if (propTransforms) {
for (const propPart in propTransforms) {
finalStorage.removeItem(
getValsKey(
id,
`${propName}.${propPart}`,
finalPersistence
)
);
}
} else {
finalStorage.removeItem(
getValsKey(id, propName, finalPersistence)
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What did this do? It's inclusion/removal doesn't seem to have an effect.

Copy link
Author

Choose a reason for hiding this comment

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

The issue with this code is that it clears the persisted value before the new selection is written via recordUiEdit, once the callback is triggered. For example, if the storage initially contains [[1, 2], []] and callback_select_all (which selects [1, 2, 3]) is triggered:

  1. The removed code clears the persistence storage prematurely which causes the original value (originalVal) to be lost, which is problematic.
  2. Then recordUiEdit writes [[1, 2, 3], [1, 2]] into storage.

This is incorrect because what should be persisted is [[1, 2, 3], []].

I might need to handle this differently. What do you think?

@gvwilson gvwilson added feature something new P1 needed for current cycle community community contribution labels Apr 22, 2025
@petar-qb
Copy link
Author

Hi @T4rk1n @alexcjohnson 👋

I just fixed the failing rdps011 with the 72cdc37.

This fix skips overwriting the persistence storage if the "persistence" object property is changed (from the callback). This makes sense as we don't want to overwrite the persisted value, but rather only to apply the persistence (from the storage to the UI component). Applying of persisted value is done within the executedCallbacks.ts in the line:

const {props} = applyPersistence({props: updatedProps}, dispatch);

Some percy/dash tests are failing but I don't understand how severe that is.

What else should be done from my side in terms of merging this PR?

@T4rk1n
Copy link
Contributor

T4rk1n commented Apr 29, 2025

This fix skips overwriting the persistence storage if the "persistence" object property is changed (from the callback). This makes sense as we don't want to overwrite the persisted value, but rather only to apply the persistence (from the storage to the UI component). Applying of persisted value is done within the executedCallbacks.ts in the line:

Thank you for fixing the test and explanation.

The percy diff are unrelated, everything looks all good. 🎉

Maybe just add a changelog then all good to go into next release 3.1

@petar-qb
Copy link
Author

@T4rk1n the changelog is added fc5eea6. Happy to see this fix merged and released soon 🥳

Thanks for all your support!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community community contribution feature something new P1 needed for current cycle
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component properties set through dynamic callbacks cannot be persisted
3 participants