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: redux sites contains functions #2757

Closed
gwwar opened this issue Jan 25, 2016 · 6 comments · Fixed by #4060
Closed

Framework: redux sites contains functions #2757

gwwar opened this issue Jan 25, 2016 · 6 comments · Fixed by #4060

Comments

@gwwar
Copy link
Contributor

gwwar commented Jan 25, 2016

The sites module in the redux state tree is not a simple object and contains references to functions. This is blocking redux state persistence in #2754

To see this, inspect reduxStore.getState().sites:
function

@aduth
Copy link
Contributor

aduth commented Jan 25, 2016

Likely a result of calling the receiveSite action with an object which has been processed by lib/sites-list to include additional handlers -

context.store.dispatch( sitesActions.receiveSite( selectedSite ) );

We should not be relying on the sites-list objects at all for the Redux state and instead store the REST API raw site response, eventually from Redux-originated action creators.

@gwwar
Copy link
Contributor Author

gwwar commented Jan 27, 2016

We also have full moment instances on sites.plans
moment

@rralian
Copy link
Contributor

rralian commented Feb 2, 2016

@gwwar Can we close this issue, since there are separate tasks to address this in the serialize/deserialize actions?

@gwwar
Copy link
Contributor Author

gwwar commented Feb 2, 2016

It's slightly different. For example currently sites are persisted, but the way the data is received and stored hasn't been cleaned up.

@rralian
Copy link
Contributor

rralian commented Mar 9, 2016

I think this issue is just a flavor of sites being one of our earliest and ugliest pieces in the data layer. I think this needs to be addressed at some point as part of regular cleanup (albeit a big cleanup task), but I'm removing it from the offline iteration.

@rralian rralian removed this from the Calypso Core: Offline 5 milestone Mar 9, 2016
@aduth
Copy link
Contributor

aduth commented Mar 10, 2016

At the very least, we should try to whitelist attributes when receiving a site into the Redux store in the immediate future. There are conflicts in doing this, as some sections of the application are already using Redux site state and expecting it to contain the computed properties. This should be a near-term focus, as it will only become more difficult with time.

I'll see if I can slice off some time in the next week to take a closer look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants