Framework: should sites be wrapped?

[gwwar]

As noted in #2757 SitesList creates 'lib/sites' or 'lib/sites/jetpack' class instances, which allows us to use methods like:

site.isCustomizable()

We are also storing these class instances in the redux sites/items tree. I'm currently scrubbing class instances down to plain objects before persisting them, but we will run into errors when someone assumes they may use any class methods after deserialization. Do we wish to continue using these class wrappers? If so I can add serialize/deserialize methods on the classes themselves.

I also considered storing plain site objects, and returning wrapped class instances from the selector, but I'm also having trouble wrapping this at the selector level due to SSR requirements for any of the theme pages https://github.com/Automattic/wp-calypso/tree/master/docs/server-side-rendering.md

As is, I'll disable persistence for this subtree until we find a good solution.

cc @aduth @mtias @rralian @retrofox @artpi @seear

[seear]

I also considered storing plain site objects, and returning wrapped class instances from the selector, but I'm also having trouble wrapping this at the selector level due to SSR requirements for any of the theme pages

This may be ok, since we will only be server-rendering logged-out theme pages, which will never involve anything to do with sites.

[artpi]

So I may have spent too much with @dmsnell recently and he may have infected me with his Reactive Functional approach, but I feel we really shouldn't store these objects.
We can provide selectors that can detect this kind of properties and memoize them, but we should not store objects because:

• redux tree should be plain objects, no exceptions (imagine having to pass objects through socket.io if we will decide on this implementation)
• there are other options to do this
• objects have 'hidden' state. This violates the "single state tree".
• objects will be prone to memory leaks. I dont know how big of a problem this is in SSR but I believe it will quickly get out of hand
• objects are more prone to errors, this misplacement, etc.

[gwwar]

This may be ok, since we will only be server-rendering logged-out theme pages, which will never involve anything to do with sites.

The UI selectors.js file is SSR ready, includes:

export function getSelectedSite( state ) {
    if ( ! state.ui.selectedSiteId ) {
        return null;
    }

    return state.sites.items[ state.ui.selectedSiteId ];
}

and that selector is currently used in client/my-sites/themes.

Would we still be able to render themes on the server side with the SSR ready removed from the selector.js file? Current build rules seem a bit strict.

[gwwar]

So I may have spent too much with @dmsnell recently and he may have infected me with his Reactive Functional approach, but I feel we really shouldn't store these objects.

I agree with you on that, though this case touches a lot of existing surface area, and redux usage would probably need to exist side by side with SitesList during the transition. I would suggest taking a look at the Site and Jetpack classes in lib/site/index and lib/site/jetpack respectively. These classes do a lot, and it may be confusing if we deal with two types of site objects.

We can of course create functions with signatures like:

isCustomizable( site );

versus

site.isCustomizable()

We'd need to be careful about keeping them in sync, or be very good about quickly removing the site class usage from the codebase.

[aduth]

An idea for interoperability, though one which I worry we might become complacent with, could be to define getters with development warnings with recommended migration paths:

Example: http://jsbin.com/goqaniyano/2/edit?js,console

[aduth]

Echoing concerns from @mtias , I think we ought to forgo interoperability, forcing any usage of site state to retrieve data via recommended practices (selectors, not custom object functions or properties), and fix any existing pages which are currently not adhering to these standards.

[mcsf]

I share the opinions that we want our data as plain and predictable as possible (an undecorated tree of normalized data, whether native or Immutable) and that it's best to put effort into fixing existing situations rather than not forcing the adoption of site state. As mentioned above, we should have all the necessary tools available (notably, selectors can be very powerful).

[rralian]

Do we wish to continue using these class wrappers?

For sure no. It's just old and pervasive and so it's a big pain to update. But it isn't this way because we still like it.

[rralian]

Closing this for now, but we can re-open or link back to it when we re-build the sites data structure.