Skip to content

Store props from callback #1900

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.

Sign up for GitHub

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 4 commits into
base: dev
Choose a base branch
from
Open

Store props from callback #1900

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82b50dd
Added function to store props from callback-generated update.
nickmelnikov82 Jan 21, 2022
bf208c4
Added persistence callback test.
nickmelnikov82 Jan 24, 2022
fe9a739
Persistence editing function merged.
nickmelnikov82 Mar 4, 2022
c0c39b5
Merge branch 'dev' into store-props-from-callback
nickmelnikov82 Apr 27, 2022
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
9 changes: 8 additions & 1 deletion dash/dash-renderer/src/observers/executedCallbacks.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,11 @@ import {ICallback, IStoredCallback} from '../types/callbacks';
import {updateProps, setPaths, handleAsyncError} from '../actions';
import {getPath, computePaths} from '../actions/paths';

import {applyPersistence, prunePersistence} from '../persistence';
import {
applyPersistence,
prunePersistence,
setPersistance
} from '../persistence';
import {IStoreObserverDefinition} from '../StoreObserver';

const observer: IStoreObserverDefinition<IStoreState> = {
Expand Down Expand Up @@ -65,6 +69,9 @@ const observer: IStoreObserverDefinition<IStoreState> = {
// those components have props to update to persist user edits.
const {props} = applyPersistence({props: updatedProps}, dispatch);

// Save props to storage if persistance is active.
setPersistance(path(itempath, layout), updatedProps, dispatch);

dispatch(
updateProps({
itempath,
Expand Down
71 changes: 49 additions & 22 deletions dash/dash-renderer/src/persistence.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,55 @@ const getProps = layout => {
};
};

export function setPersistance(layout, newProps, dispatch) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@nickmelnikov82 my apologies for not reviewing this quicker!

Can you explain the differences between this new function and recordUiEdit? Ideally we'd 🌴 combine them into one (which we could call recordEdit I guess) with just a flag to distinguish the two situations, then any differences would be crystal clear.

The differences I see:

  • Here you needed to create finalPersistence - makes sense because the UI can't edit persistence - but in fact I don't see a harm in including this in recordUiEdit, it's not a big overhead. Which brings up that in the callback case it's also possible to alter persistence_type, so presumably we should also create finalPersistenceType.
  • Why the short-circuit on finalPersistence !== persistence? I guess maybe it's more complicated in this case, at least if you change from one truthy persistence value to another, but it seems to me like we'd still want the new value stored.
  • Can you explain the storage.removeItem case? Is that situation accessible from the UI as well (ie a case we were always missing)?
  • recordUiEdit has const vals = originalVal === undefined ? [newVal] : [newVal, originalVal]; - ie we don't store undefined. I'm not sure the practical consequences of this, but presumably we want to keep that behavior?

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. About finalPersistenceType it makes sense
  2. Yes, it's related to case when one truthy persistence value to another. When persistence is changed, the current value is immediately written to the storage as the original value.
  3. This is for the case described in the documentation
  4. Without these lines, more than half of the tests fail. I'm not quite sure how storage without the original value is used.

const {
canPersist,
id,
props,
element,
persistence,
persisted_props,
persistence_type
} = getProps(layout);

const getFinal = (propName, prevVal) =>
propName in newProps ? newProps[propName] : prevVal;
const finalPersistence = getFinal('persistence', persistence);

if (!canPersist || !finalPersistence || finalPersistence !== persistence) {
return;
}

forEach(persistedProp => {
const [propName, propPart] = persistedProp.split('.');
if (newProps[propName] !== undefined) {
const storage = getStore(persistence_type, dispatch);
const {extract} = getTransform(element, propName, propPart);

const valsKey = getValsKey(id, persistedProp, persistence);
let originalVal = extract(props[propName]);
const newVal = extract(newProps[propName]);

if (originalVal !== newVal) {
if (storage.hasItem(valsKey)) {
originalVal = storage.getItem(valsKey)[1];
if (newVal !== originalVal) {
storage.setItem(
valsKey,
[newVal, originalVal],
dispatch
);
} else {
storage.removeItem(valsKey);
}
} else {
storage.setItem(valsKey, [newVal, originalVal], dispatch);
}
}
}
}, persisted_props);
}

export function recordUiEdit(layout, newProps, dispatch) {
const {
canPersist,
Expand Down Expand Up @@ -517,28 +566,6 @@ export function prunePersistence(layout, newProps, dispatch) {
filter(notInNewProps, finalPersistedProps)
);
}

// now the main point - clear any edit of a prop that changed
// note that this is independent of the new prop value.
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)
);
}
}
}
return persistenceChanged ? mergeRight(newProps, update) : newProps;
}
61 changes: 61 additions & 0 deletions tests/integration/renderer/test_persistence.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,3 +558,64 @@ def update_container2(n_clicks):
# persistenceTransforms should return upper case strings
dash_duo.wait_for_text_to_equal("#component-propName", "ALPACA")
dash_duo.wait_for_text_to_equal("#component-propPart", "ARTICHOKE")


def test_rdps014_save_callback_persistence(dash_duo):
app = dash.Dash(__name__)

def make_input(persistence):
return dcc.Input(id="persisted", value="a", persistence=persistence)

app.layout = html.Div(
[
dcc.Input(id="persistence-val", value=""),
dcc.Input(id="persistence-key", value=""),
html.Div(make_input(""), id="persisted-container"),
html.Div(id="out"),
]
)

# this is not a good way to set persistence, as it doesn't allow you to
# get the right initial value. Much better is to update the whole component
# as we do in the previous test case... but it shouldn't break this way.
@app.callback(
Output("persisted", "persistence"), [Input("persistence-key", "value")]
)
def set_persistence(val):
return val

@app.callback(Output("persisted", "value"), [Input("persistence-val", "value")])
def set_value(val):
return val

@app.callback(Output("out", "children"), [Input("persisted", "value")])
def set_out(val):
return val

dash_duo.start_server(app)

dash_duo.wait_for_text_to_equal("#out", "")

dash_duo.find_element("#persistence-key").send_keys("a")
time.sleep(0.2)
assert not dash_duo.get_logs()
dash_duo.wait_for_text_to_equal("#out", "")
dash_duo.find_element("#persistence-val").send_keys("alpaca")
dash_duo.wait_for_text_to_equal("#out", "alpaca")

dash_duo.find_element("#persistence-key").send_keys("b")
dash_duo.wait_for_text_to_equal("#out", "")
dash_duo.clear_input("#persistence-val")
dash_duo.find_element("#persistence-val").send_keys("artichoke")
dash_duo.wait_for_text_to_equal("#out", "artichoke")

# no persistence, still goes back to original value
dash_duo.clear_input("#persistence-key")
dash_duo.wait_for_text_to_equal("#out", "")

# apricot and artichoke saved
dash_duo.find_element("#persistence-key").send_keys("a")
dash_duo.wait_for_text_to_equal("#out", "alpaca")
dash_duo.find_element("#persistence-key").send_keys("b")
assert not dash_duo.get_logs()
dash_duo.wait_for_text_to_equal("#out", "artichoke")