Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Make Store component more robust #792

Merged
merged 6 commits into from
May 5, 2020
Merged

Make Store component more robust #792

merged 6 commits into from
May 5, 2020

Conversation

alexcjohnson
Copy link
Collaborator

@alexcjohnson alexcjohnson commented Apr 13, 2020

Fixes #456
Fixes #778 (unless something else already fixed that)

In particular these fixes support unmounting & remounting Store components, and various data edge cases

This should allow us to remove the confusing special handling of Store in the docs.

in particular to support unmounting & remounting it and various data edge cases
@@ -150,7 +117,7 @@ export default class Store extends React.Component {
}

const old = this._backstore.getItem(id);
if (isNil(old) && data) {
if (isNil(old) && !isNil(data)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

// out the old value.
// Note: this still allows you to set data to null
if (!equals(data, old)) {
if (data === undefined) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I bet this would all be even simpler if we moved to a functional component with useEffect, but anyway this is the key point allowing you to re-supply the same component with no data and have the old value persist.

If you actually remove it completely and bring it back later, it already worked, via the UNSAFE_componentWillMount pathway. This is only if the component is already on the page and you provide it again - ie as part of a larger children prop in which some other things may have changed.

// Note: this still allows you to set data to null
if (!equals(data, old)) {
if (data === undefined) {
setProps({data: old});
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Apr 15, 2020

Choose a reason for hiding this comment

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

Running this against a modified version of https://dash.plotly.com/dash-core-components/store, I expected to be able to rename the dcc.Store component in there for ones that were unique to this layout and everything work 👌

Turns out the callback triggered by https://github.com/plotly/dash-docs/blob/master/dash_docs/chapters/dash_core_components/Store/examples/store_clicks.py#L74 is not triggered on load, resulting in all field having value 0. On click, they are updated to previous+1 as expected.

Possibly outside the scope of this PR, but a close neighbour.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

is not triggered on load, resulting in all field having value 0.

Can you post the exact app you're using? The app seems to behave correctly when I try it.

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Apr 29, 2020

Choose a reason for hiding this comment

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

Running from the docs entry point index.py:

Prior to changes,

  1. Opening http://127.0.0.1:8060/dash-core-components/store, all values for the stores will be 0. Click the buttons to update and see that the callbacks work.
  2. Refresh the page, local and session storage counters are restore to their value, memory is 0

Here's the modified code for store_clicks.py

  • remove # no-exec comments
  • rename memory/local/session to new-[memory|session|local]
  • update callbacks accordingly
import dash

import dash_html_components as html
import dash_core_components as dcc
from dash.dependencies import Output, Input, State
from dash.exceptions import PreventUpdate

# This stylesheet makes the buttons and table pretty.
external_stylesheets = ['https://codepen.io/chriddyp/pen/bWLwgP.css']

app = dash.Dash(__name__, external_stylesheets=external_stylesheets)

app.layout = html.Div([
    # The memory store reverts to the default on every page refresh
    dcc.Store(id='new-memory'), # stores are loaded from the beginning
    # The local store will take the initial data
    # only the first time the page is loaded
    # and keep it until it is cleared.
    dcc.Store(id='new-local', storage_type='local'),
    # Same as the local store but will lose the data
    # when the browser/tab closes.
    dcc.Store(id='new-session', storage_type='session'),
    html.Table([
        html.Thead([
            html.Tr(html.Th('Click to store in:', colSpan="3")),
            html.Tr([
                html.Th(html.Button('memory', id='memory-button')),
                html.Th(html.Button('localStorage', id='local-button')),
                html.Th(html.Button('sessionStorage', id='session-button'))
            ]),
            html.Tr([
                html.Th('Memory clicks'),
                html.Th('Local clicks'),
                html.Th('Session clicks')
            ])
        ]),
        html.Tbody([
            html.Tr([
                html.Td(0, id='memory-clicks'),
                html.Td(0, id='local-clicks'),
                html.Td(0, id='session-clicks')
            ])
        ])
    ])
])

# Create two callback for every store.
for store in ('memory', 'local', 'session'):

    # add a click to the appropriate store.
    @app.callback(Output('new-{}'.format(store), 'data'),
                  [Input('{}-button'.format(store), 'n_clicks')],
                  [State('new-{}'.format(store), 'data')])
    def on_click(n_clicks, data):
        if n_clicks is None:
            # prevent the None callbacks is important with the store component.
            # you don't want to update the store for nothing.
            raise PreventUpdate

        # Give a default data dict with 0 clicks if there's no data.
        data = data or {'clicks': 0}

        data['clicks'] = data['clicks'] + 1
        return data

    # output the stored clicks in the table cell.
    @app.callback(Output('{}-clicks'.format(store), 'children'),
                  # Since we use the data prop in an output,
                  # we cannot get the initial data on load with the data prop.
                  # To counter this, you can use the modified_timestamp
                  # as Input and the data as State.
                  # This limitation is due to the initial None callbacks
                  # https://github.com/plotly/dash-renderer/pull/81
                  [Input('new-{}'.format(store), 'modified_timestamp')],
                  [State('new-{}'.format(store), 'data')])
    def on_data(ts, data):
        if ts is None:
            raise PreventUpdate

        data = data or {}

        return data.get('clicks', 0)


if __name__ == '__main__':
    app.run_server(debug=True, port=8077, threaded=True)
  1. Opening http://127.0.0.1:8060/dash-core-components/store, all values for the stores will be 0. Click the buttons to update and see that the callbacks work.
  2. Refresh the page, all counters are at 0 again
  3. Click on the local or session counter button, notice that it will jump from 0 to previous+1

Running this against latest of dash@dev.


Running store_clicks.py directly, not from inside the entire docs app, the behavior is as expected.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

got it, thanks! Investigating... Here's a standalone version that shows the bug, just wrapping the layout in a content callback:

import dash

import dash_html_components as html
import dash_core_components as dcc  # noxx-exec
from dash.dependencies import Output, Input, State
from dash.exceptions import PreventUpdate

# This stylesheet makes the buttons and table pretty.
external_stylesheets = ['https://codepen.io/chriddyp/pen/bWLwgP.css']

app = dash.Dash(
    __name__,
    external_stylesheets=external_stylesheets,
    suppress_callback_exceptions=True
)

layout = html.Div([
    # The memory store reverts to the default on every page refresh
    dcc.Store(id='memory'),  # noxx-exec - stores are loaded from the beginning
    # The local store will take the initial data
    # only the first time the page is loaded
    # and keep it until it is cleared.
    dcc.Store(id='local', storage_type='local'),  # noxx-exec
    # Same as the local store but will lose the data
    # when the browser/tab closes.
    dcc.Store(id='session', storage_type='session'),  # noxx-exec
    html.Table([
        html.Thead([
            html.Tr(html.Th('Click to store in:', colSpan="3")),
            html.Tr([
                html.Th(html.Button('memory', id='memory-button')),
                html.Th(html.Button('localStorage', id='local-button')),
                html.Th(html.Button('sessionStorage', id='session-button'))
            ]),
            html.Tr([
                html.Th('Memory clicks'),
                html.Th('Local clicks'),
                html.Th('Session clicks')
            ])
        ]),
        html.Tbody([
            html.Tr([
                html.Td(0, id='memory-clicks'),
                html.Td(0, id='local-clicks'),
                html.Td(0, id='session-clicks')
            ])
        ])
    ])
])

app.layout = html.Div(id="content")


@app.callback(Output("content", "children"), [Input("content", "style")])
def content(_):
    return layout


# Create two callback for every store.
for store in ('memory', 'local', 'session'):
    # add a click to the appropriate store.
    @app.callback(Output(store, 'data'),
                  [Input('{}-button'.format(store), 'n_clicks')],
                  [State(store, 'data')])
    def on_click(n_clicks, data):
        if n_clicks is None:
            # prevent the None callbacks is important with the store component.
            # you don't want to update the store for nothing.
            raise PreventUpdate

        # Give a default data dict with 0 clicks if there's no data.
        data = data or {'clicks': 0}

        data['clicks'] = data['clicks'] + 1
        return data

    # output the stored clicks in the table cell.
    @app.callback(Output('{}-clicks'.format(store), 'children'),
                  # Since we use the data prop in an output,
                  # we cannot get the initial data on load with the data prop.
                  # To counter this, you can use the modified_timestamp
                  # as Input and the data as State.
                  # This limitation is due to the initial None callbacks
                  # https://github.com/plotly/dash-renderer/pull/81
                  [Input(store, 'modified_timestamp')],
                  [State(store, 'data')])
    def on_data(ts, data):
        if ts is None:
            raise PreventUpdate

        data = data or {}

        return data.get('clicks', 0)


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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ha, this is actually yet another renderer bug... PR coming in Dash as soon as I figure out how to write a test for it!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in plotly/dash#1224

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

https://github.com/plotly/dash-docs/pull/871 shows that between this PR and plotly/dash#1224 there are no issues anymore running the store examples unmodified within the docs.

@Marc-Andre-Rivet
Copy link
Contributor

@alexcjohnson Took the liberty of updating the PR d771fc7 for side-effect of the dash-renderer build that uncovered the missing npm ci when building the renderer from dev in CI.

@wbrgss
Copy link
Contributor

wbrgss commented May 4, 2020

Took the liberty of updating the PR d771fc7 for side-effect of the dash-renderer build that uncovered the missing npm ci when building the renderer from dev in CI.

Nice, thanks @Marc-Andre-Rivet! Depending on how close (this PR) is, d771fc7 might warrant its own PR. DCC (among other libraries) build dash-renderer in their CI runs, so sh: 1: babel: not found is blocking these runs at the moment.

E.g. https://circleci.com/gh/plotly/dash-core-components/20126

@Marc-Andre-Rivet
Copy link
Contributor

@wbrgss Afaik, this is ready to go. plotly/dash-table#766 fixes it for the table, the html component fix is already in. Since these should all the last items to make it into Dash v1.12 prior to the fix, it's probably OK to just wait it out. Let me know if you have some additional timing concerns and I'll make a side PR for table/dcc.

@alexcjohnson alexcjohnson merged commit 4b1f9cb into dev May 5, 2020
@alexcjohnson alexcjohnson deleted the store-robustify branch May 5, 2020 00:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Undesired behaviour (interaction?) with two dcc.Store issue with dcc.Store('local') ?
3 participants