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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
be2f01f
Make Store component more robust
alexcjohnson Apr 13, 2020
5b0b5de
lint
alexcjohnson Apr 13, 2020
18af043
Merge branch 'dev' into store-robustify
alexcjohnson May 2, 2020
c3e0493
changelog for dcc.Store fixes
alexcjohnson May 2, 2020
2b2d6fa
Merge branch 'dev' into store-robustify
Marc-Andre-Rivet May 4, 2020
d771fc7
Do `npm ci` on local build of dash-renderer
Marc-Andre-Rivet May 4, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
71 changes: 23 additions & 48 deletions src/components/Store.react.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,46 +1,7 @@
import {any, includes, isNil, type} from 'ramda';
import {equals, isNil} from 'ramda';
import React from 'react';
import PropTypes from 'prop-types';

/**
* Deep equality check to know if the data has changed.
*
* @param {*} newData - New data to compare
* @param {*} oldData - The old data to compare
* @returns {boolean} The data has changed.
*/
function dataChanged(newData, oldData) {
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved
const oldNull = isNil(oldData);
const newNull = isNil(newData);
if (oldNull || newNull) {
return newData !== oldData;
}
const newType = type(newData);
if (newType !== type(oldData)) {
return true;
}
if (newType === 'Array') {
if (newData.length !== oldData.length) {
return true;
}
for (let i = 0; i < newData.length; i++) {
if (dataChanged(newData[i], oldData[i])) {
return true;
}
}
} else if (includes(newType, ['String', 'Number', 'Boolean'])) {
return oldData !== newData;
} else if (newType === 'Object') {
const oldEntries = Object.entries(oldData);
const newEntries = Object.entries(newData);
if (oldEntries.length !== newEntries.length) {
return true;
}
return any(([k, v]) => dataChanged(v, oldData[k]))(newEntries);
}
return false;
}

/**
* Abstraction for the memory storage_type to work the same way as local/session
*
Expand Down Expand Up @@ -88,7 +49,13 @@ class WebStore {
}

getItem(key) {
return JSON.parse(this._storage.getItem(key));
try {
return JSON.parse(this._storage.getItem(key));
} catch (e) {
// in case we somehow got a non-JSON value in storage,
// just ignore it.
return null;
alexcjohnson marked this conversation as resolved.
Show resolved Hide resolved
}
}

setItem(key, value) {
Expand Down Expand Up @@ -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.

test_stcp003_initial_falsy

// Initial data mount
this._backstore.setItem(id, data);
setProps({
Expand All @@ -159,7 +126,7 @@ export default class Store extends React.Component {
return;
}

if (dataChanged(old, data)) {
if (!equals(old, data)) {
setProps({
data: old,
modified_timestamp: this._backstore.getModified(id),
Expand All @@ -186,11 +153,19 @@ export default class Store extends React.Component {
}
const old = this._backstore.getItem(id);
// Only set the data if it's not the same data.
if (dataChanged(data, old)) {
this._backstore.setItem(id, data);
setProps({
modified_timestamp: this._backstore.getModified(id),
});
// If the new data is undefined, we got here by overwriting the entire
// component with a new copy that has no `data` specified - so pull back
// 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.

setProps({data: old});
Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet Apr 15, 2020 •
edited
Loading

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 •
edited
Loading

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.

} else {
this._backstore.setItem(id, data);
setProps({
modified_timestamp: this._backstore.getModified(id),
});
}
}
}

Expand Down
120 changes: 120 additions & 0 deletions tests/integration/store/test_component_props.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,3 +70,123 @@ def init_output(ts, data):
assert ts == approx(
output_data.get("ts"), abs=40
), "the modified_timestamp should be updated right after the click action"


def test_stcp003_initial_falsy(dash_dcc):
app = dash.Dash(__name__)
app.layout = html.Div([html.Div([
storage_type,
dcc.Store(storage_type=storage_type, id="zero-" + storage_type, data=0),
dcc.Store(storage_type=storage_type, id="false-" + storage_type, data=False),
dcc.Store(storage_type=storage_type, id="null-" + storage_type, data=None),
dcc.Store(storage_type=storage_type, id="empty-" + storage_type, data=""),
]) for storage_type in ("memory", "local", "session")], id="content")

dash_dcc.start_server(app)
dash_dcc.wait_for_text_to_equal("#content", "memory\nlocal\nsession")

for storage_type in ("local", "session"):
getter = getattr(dash_dcc, "get_{}_storage".format(storage_type))
assert getter("zero-" + storage_type) == 0, storage_type
assert getter("false-" + storage_type) is False, storage_type
assert getter("null-" + storage_type) is None, storage_type
assert getter("empty-" + storage_type) == "", storage_type

assert not dash_dcc.get_logs()


def test_stcp004_remount_store_component(dash_dcc):
app = dash.Dash(__name__, suppress_callback_exceptions=True)

content = html.Div(
[
dcc.Store(id="memory", storage_type="memory"),
dcc.Store(id="local", storage_type="local"),
dcc.Store(id="session", storage_type="session"),
html.Button("click me", id="btn"),
html.Button("clear data", id="clear-btn"),
html.Div(id="output"),
]
)

app.layout = html.Div([html.Button("start", id="start"), html.Div(id="content")])

@app.callback(Output("content", "children"), [Input("start", "n_clicks")])
def start(n):
return content if n else "init"

@app.callback(
Output("output", "children"),
[
Input("memory", "modified_timestamp"),
Input("local", "modified_timestamp"),
Input("session", "modified_timestamp")
],
[State("memory", "data"), State("local", "data"), State("session", "data")],
)
def write_memory(tsm, tsl, tss, datam, datal, datas):
return json.dumps([datam, datal, datas])

@app.callback(
[
Output("local", "clear_data"),
Output("memory", "clear_data"),
Output("session", "clear_data"),
],
[Input("clear-btn", "n_clicks")],
)
def on_clear(n_clicks):
if n_clicks is None:
raise PreventUpdate
return True, True, True

@app.callback(
[
Output("memory", "data"),
Output("local", "data"),
Output("session", "data"),
],
[Input("btn", "n_clicks")],
)
def on_click(n_clicks):
return ({"n_clicks": n_clicks},) * 3

dash_dcc.start_server(app)

dash_dcc.wait_for_text_to_equal("#content", "init")

dash_dcc.find_element("#start").click()
dash_dcc.wait_for_text_to_equal(
"#output",
'[{"n_clicks": null}, {"n_clicks": null}, {"n_clicks": null}]'
)

dash_dcc.find_element("#btn").click()
dash_dcc.wait_for_text_to_equal(
"#output",
'[{"n_clicks": 1}, {"n_clicks": 1}, {"n_clicks": 1}]'
)

dash_dcc.find_element("#clear-btn").click()
dash_dcc.wait_for_text_to_equal("#output", "[null, null, null]")

dash_dcc.find_element("#btn").click()
dash_dcc.wait_for_text_to_equal(
"#output",
'[{"n_clicks": 2}, {"n_clicks": 2}, {"n_clicks": 2}]'
)

# now remount content components
dash_dcc.find_element("#start").click()
dash_dcc.wait_for_text_to_equal(
"#output",
'[{"n_clicks": null}, {"n_clicks": null}, {"n_clicks": null}]'
)

dash_dcc.find_element("#btn").click()
dash_dcc.wait_for_text_to_equal(
"#output",
'[{"n_clicks": 1}, {"n_clicks": 1}, {"n_clicks": 1}]'
)

assert not dash_dcc.get_logs()