Skip to content
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

Framework: Refuse to persist any UI state #3337

Merged
merged 2 commits into from
Feb 25, 2016

Conversation

aduth
Copy link
Contributor

@aduth aduth commented Feb 16, 2016

This pull request seeks to update the base state.ui reducer to always return an empty object on the DESERIALIZE and SERIALIZE action types, thereby preventing any state contained within from persisting or restoring persisted state.

Open questions:

Is this desirable? Effectively, this treats UI state as disposable to the extent that it should not be restored upon revisiting the application. This seems reasonable (arguably required) in the web client, but is more debatable in the context of the desktop application. We should ask ourselves whether it seems reasonable that UI state be persisted between sessions of using the desktop application.

/cc @gwwar , @mtias

@aduth aduth added [Type] Question Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 16, 2016
@aduth
Copy link
Contributor Author

aduth commented Feb 16, 2016

Failing tests seem to be partly related to intermingling server state hydration with persisted state deserialization. We'd probably want to rethink this if we decide to move forward with these changes.

@gwwar
Copy link
Contributor

gwwar commented Feb 16, 2016

It does seem our concerns are mixing a bit with regards to server initialization. Maybe we should be a bit more explicit about this. We could do an unbalanced serialize that only returns an object, and a deserialize method that still attempts to hydrate state. Alternatively there could be a separate hydration pipeline for server state.

cc @seear

@seear
Copy link
Contributor

seear commented Feb 16, 2016

Alternatively there could be a separate hydration pipeline for server state.

This would be perfectly fine. The only reason for passing bootstrapped server state through the deserializer is to take advantage of the conversion to immutable data. That can be done elsewhere to keep bootstrapping separate from deserialization.

@gwwar
Copy link
Contributor

gwwar commented Feb 16, 2016

This would be perfectly fine.

👍 Added a quick approach for this in #3343

@gwwar
Copy link
Contributor

gwwar commented Feb 19, 2016

@aduth since #3343 landed, ping me when you rebase this branch, and I can help update the initial-state.js test to use a different subtree than ui.

@gwwar
Copy link
Contributor

gwwar commented Feb 24, 2016

👍 I don't believe we have any blockers left. 🚢

@gwwar gwwar added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 24, 2016
@aduth aduth force-pushed the update/redux-prevent-ui-persist branch from 6f0e22a to b03a15e Compare February 25, 2016 15:59
@aduth
Copy link
Contributor Author

aduth commented Feb 25, 2016

Rebased to account for additional UI state added in #3430 .

Will plan to merge shortly.

aduth added a commit that referenced this pull request Feb 25, 2016
@aduth aduth merged commit 10bd703 into master Feb 25, 2016
@aduth aduth deleted the update/redux-prevent-ui-persist branch February 25, 2016 16:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants