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: add approach for redux data persistence #3267

Merged
merged 1 commit into from
Feb 24, 2016

Conversation

gwwar
Copy link
Contributor

@gwwar gwwar commented Feb 11, 2016

Data persistence is still a wip but I think we should update this as we go along.

cc @rralian @mtias @aduth @blowery @dmsnell @ockham

@gwwar gwwar added Framework [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 11, 2016
@gwwar gwwar self-assigned this Feb 11, 2016
reducer( browserState, { type: 'DESERIALIZE' } )
```

The key idea is that in the reducer `SERIALIZE` should return a plain JS object. In a subtree that uses Immutable.js
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we explain why it's important to return a JavaScript object?

Because browser storage is only capable of storing simple JavaScript objects, the purpose of the `SERIALIZE` action type reducer handler is to return a plain object representation.

@aduth
Copy link
Contributor

aduth commented Feb 12, 2016

The headings are a bit confusing to the flow of reading:

  • Persistence
    • Problem
    • Problem
    • Solution
    • Solution
  • Not persisting

I might expect it to be structured as:

  • Persistence
    • Problems
      • Problem
        • Solution
      • Problem
        • Solution
    • Not persisting

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Feb 12, 2016
@gwwar gwwar force-pushed the add/persist-redux-documentation branch 7 times, most recently from 289dd21 to 7057d5a Compare February 12, 2016 20:15
@gwwar
Copy link
Contributor Author

gwwar commented Feb 12, 2016

Thanks for the 👀 @aduth! Great suggestions. I hopefully addressed most of the issues in 7057d5a I opted to not go more than 2 deep with headers, because the styling looks a bit odd.

@gwwar gwwar added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] Needs Author Reply labels Feb 12, 2016
@gwwar gwwar added this to the Calypso Core: Offline 4 milestone Feb 12, 2016
@gwwar gwwar force-pushed the add/persist-redux-documentation branch 2 times, most recently from 0e5671e to 67dc83a Compare February 24, 2016 18:22
@gwwar gwwar mentioned this pull request Feb 24, 2016
@rralian
Copy link
Contributor

rralian commented Feb 24, 2016

I think this looks great... thanks for writing it up! Looks good to 🚢 to me.

@rralian rralian 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
@gwwar
Copy link
Contributor Author

gwwar commented Feb 24, 2016

Thanks for the reviews! Going to squash commits.

@gwwar gwwar force-pushed the add/persist-redux-documentation branch from 67dc83a to e22a61b Compare February 24, 2016 21:40
gwwar added a commit that referenced this pull request Feb 24, 2016
Framework: add approach for redux data persistence
@gwwar gwwar merged commit b39baef into master Feb 24, 2016
@gwwar gwwar deleted the add/persist-redux-documentation branch February 24, 2016 22:02
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