Docs: Include documentation on "Our Approach to Data"

[aduth]

This pull request seeks to add a new document to our docs directory, with the purpose of outlining a history of our approaches to data, and to prescribe our current set of recommendations.

See Full Document

[rralian]

This is fantastic!... really well-written and considered. Thanks @aduth and others who helped talk this through. Our own contributors have been asking for this for a long time -- "It's hard to know which patterns to follow and which to avoid."

Question about this.

Use your best judgment when considering whether to add to the global state, but don't feel compelled to avoid React state altogether.

Is there a downside to tracking all state in the global state object? I suppose that would be pretty complex, considering you would need to track state against each individual component. I just get excited about the idea of these state objects helping us to debug problems in the application (by grabbing the object from an exception log or a help chat session). And that would be reduced somewhat by having some state that is not tracked globally... we couldn't exactly replicate what was going on in the browser.

[rralian]

Wouldn't selectors make sense for retrieving computed properties?

[aduth]

Wouldn't selectors make sense for retrieving computed properties?

I've intentionally left this section as WIP because I'm not really sold one way or the other yet. I also think this potentially leads us to other questions such as: Should we "fix" API responses to a format consistent with our naming guidelines (i.e. site.ID -> site.id).

The particular question that was posed was: Developers currently have easy access to a site domain by calling site.domain. The domain is a derived property based on other fields on the object. What recommendation should we have for access this data moving forward?

A few options I see are:

// 1)
// 
// Components should receive the most minimal data necessary, so pass `domain`
// using a selector which calculates the value based on the site from state tree

export default connect( ( state ) => {
    return {
        url: getSite( state, siteId ).URL,
        domain: getSiteDomain( state, siteId )
    };
} )( SiteLink );

// Pro: Component receives minimal props
// Con: Domain is not as accessible as calling to "site.domain"

// 2)
// 
// Components should receive the minimal set of props, so pass domain, but as
// a destructured property of the site object stored in state

export default connect( ( state, ownProps ) => {
    const { URL: url, domain } = getSite( state, ownProps.siteId );
    return { url, domain };
} )( SiteLink );

// Pro: As accessible as calling to "site.domain"
// Pro: Component receives minimal props
// Con: Either the site state is not normalized (has redundant computed data),
//      or we must define JavaScript getters in the state tree

// 3)
// 
// Pass the entire site object, including derived properties, to the component

export default connect( ( state, ownProps ) => {
    return { site: getSite( state, ownProps.siteId ) };
} )( SiteLink );

// Pro: Easy to access any necessary values from component itself
// Con: Not passing minimal set of props
// Con: Site not normalized or JavaScript getters in state tree

[aduth]

Is there a downside to tracking all state in the global state object?

I expect there to be some debate here 😄 It seemed to come down to weighing the benefits against the costs in terms of practicality. I'd agree with you that it'd be excellent for debugging if we could simply "export" the entire state of the application at any given moment. However, in practice, I think we'll find that it requires jumping through hoops to make it work, with particular concerns around:

• Where does the interface state live in the tree?
  • At the root ui key? Will that scale?
  • Aligned to components or to routes? What if a component is reused in different contexts?
• When does the interface state get cleared?
  • In creating a reducer for this state, am I responsible for clearing it in response to a route change?

It's also a significant amount of code to do well - implementing and testing a reducer, actions, action types, etc, all for a simple toggle on a component? Why go to such great lengths to avoid using a feature of React that's afforded to us for this very purpose - to maintain the state of a particular piece of the render hierarchy for the duration of the time that it's mounted?

Playing devil's advocate to myself:

• I foresee a possible future where React only includes stateless function components, and has no concept of state built-in.
• It may be wiser for us to be consistent with how we store data, UI or otherwise.

[ockham]

The client/state directory contains all of the behavior describing the global application state.

Shouldn't this be shared/state, so we can use it server-side?

[mtias]

I really want to avoid more fragmentation of /shared | /client. Can we figure out a plan for it?

[ockham]

Not sure about the implications for node and webpack -- @ehg and @seear will know better.

Other than that, I guess we'll still need some way to preserve components as usable-on-server though, so they aren't accidentally broken e.g. by non-team committers. We might want to turn some of https://github.com/Automattic/wp-calypso/blob/master/shared/README.md into general guidelines that are applicable to client-only components, and a subset for components that are meant to be used on the server side, too.

Writing tests to ensure components are still working server-side is going to be quite tedious :-/

CC @ehg @seear @mcsf

[apeatling]

@nb What do you think about this? Think we can we merge this doc?

[aduth]

As evidenced by recent commits, some of the more specific guidelines are still unsettled, subject to changes through experimentation. I'd like to have at least some form of this document merged to master in the near future, so perhaps we decide to either (a) remove specific guidelines or (b) be content that they're in flux, and future revisions can be made through subsequent pull requests.

[apeatling]

👍

[rralian]

Is this.translate() available to stateless function components? I didn't think it was.

[mtias]

It's not.

[aduth]

Is this.translate() available to stateless function components? I didn't think it was.

You're correct, it's not available. Updated to use React.Component class instead in e6611aa.

[rralian]

I'd like to have at least some form of this document merged to master in the near future, so perhaps we decide to either (a) remove specific guidelines or (b) be content that they're in flux, and future revisions can be made through subsequent pull requests.

I vote b. Let's merge this. I had one concern that I don't think you can use the translate mixin from a stateless function, so (unless I'm wrong) it'd be nice to change that to use React.createClass. But otherwise I think we should merge this. 👍

[mtias]

I think we should revise this with the latest addition of Query components.

[aduth]

I think we should revise this with the latest addition of Query components.

Anything specifically that you're finding to be inaccurate? There were revisions in 6cc3c69 to reflect that we should not be using the existence of data to determine whether fetching should occur.

[mtias]

I don't consider Query components Container Components in the way we have fetching components wrapping UI/app components and I think it would be a bit confusing to have so many concepts around them.

[aduth]

@mtias : Pushed dc3cec5 with some revised terminology with the goal of removing any distinctions between "app components" and "connected components", or between "fetching components" and "query components". Since you're correct that query components aren't really a container component, I've renamed the major section to "Data Components".

[mtias]

💥