Exporter: Move data into the Redux store

[jordwest]

This PR moves the Exporter component state out of the React component and into a reducerStore the global Redux store. This will simplify data management as the exporter component develops.

cc @dllh @dmsnell

[dllh]

Is there a useful way to test this or is it likely to be pretty much a pure code review?

I'll leave others who've done stuff with stores to review the code. With Thanksgiving going on in the U.S. right now, it might be good to find a non-U.S. review buddy for the second half of the week to avoid being blocked.

[dllh]

Maybe use the debug require for this rather than doing a direct console.log()?

[jordwest]

@dllh There are no functionality changes in this PR, so it will be a pure code review

[nb]

Do you think it will be an overkill if we make AdvancedOptions connected to the store, too? This way we won't need to pass it down the chain, we will be able to make this component dependent only on isVIsible.

[jordwest]

@nb: I was thinking along similar lines - it seems like it would be neater. But then I came across this in the Redux docs:

It is advisable that only top-level components of your app (such as route handlers) are aware of Redux. Components below them should be presentational and receive all data via props.

Which gives the impression that eventually only the very top level component should be aware of the store, and everything gets passed down?

[nb]

This makes perfect sense in a case where the depth is 2 or 3, but with more complicated UIs to me it makes less and less sense, both because more independent components emerge, and because we seem to be passing data that’s not obvious how we need.

I have an idea that, in my mind, looks more logical:

• We merge Exporter and ExporterContainer
• We extract the CompactCard block into BasicSettings

Reasoning:

• I seriously doubt we will reuse the exporter without the need of the data connection, but keeping them together save us a level of nesting and redirection (even if we need at some point, extraction is trivial).
• Now both of the settings components will only get the data they need and we won’t have the Exporter, which gets all of the settings only to pass them to a child.

P.S. Don’t treat this as a blocker, it’s more of a philosophical exploration for going forward.
P.P.S. Why do we have options and settings? Do they mean the same thing?

[aduth]

I might dissent a bit here. While I'd agree that it doesn't seem important that the data components necessarily live at the top of the component hierarchy, I do like the idea of a hard rule to keep data and presentational components separate.

You may already be familiar with these articles (and they're linked in the docs mentioned above); they do well to highlight the need for and purpose of splitting container components from their visual counterpart:

• https://medium.com/@learnreact/container-components-c0e67432e005
• https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0

My main takeaways here are that the following gains are to be had by always keeping "smart" and "dumb" components separate:

• Reusability: Separating data from presentation enables each to be portable. The visual component can be rendered in any context so long as its data needs are satisfied, regardless of those source of that data.
• Clarity: If merged, I'd be wasting effort deciphering where the data management ends and the visual handling begins. Single-purpose components tend to be smaller and easier to digest. We shouldn't be afraid to create and nest components.
• Development ease: Simple-to-follow rules can eliminate the time a developer would otherwise spend evaluating when and how to separate the two. Predictable patterns enable outside developers to quickly grok the purpose of the components.

(The grain-of-salt to take here is that I have a tendency to be quick to abstract, though I've tried my best to account for YAGNI in the reasoning above.)

The issue I have with the implementation here is that we should do our best to be as granular as possible with what we're sending down to child components. In hind-sight, this is something that I had wished we'd done a better job of managing props in the post editor (ref: p4TIVU-11B-p2). Being more granular and making a best effort to send non-complex props makes it much easier to achieve pure components, improves reusability, and clearly identifies the needs of the component. Rather than passing a large advancedSettings object through the hierarchy here, I think it'd be better to pick off and pass only the required data in rendering each component.

Related docs:

• https://facebook.github.io/react/docs/thinking-in-react.html#step-3-identify-the-minimal-but-complete-representation-of-ui-state
• https://facebook.github.io/react/docs/pure-render-mixin.html

[aduth]

I need to run, but I'd like to expand more on this point later: "Being more granular and making a best effort to send non-complex props makes it much easier to achieve pure components, improves reusability, and clearly identifies the needs of the component." There are several occurrences in the post editor where reliance on passing large complex objects has made building new components much more difficult than it needs to be.

[nb]

@aduth, when you get back, I would be curious what your proposed structure for this code would be?

[aduth]

Here's a quick and rough outline of how I might expect the component tree to look here:

https://gist.github.com/aduth/2176ebbd8cc87a231a4f

I'm not entirely familiar with the needs and behavior of the section dropdowns themselves, so the approach here may be a bit naively simple, but at least goes so far as to demonstrate the idea behind granularity of props, keeping container components tailored to providing the minimal set of props it needs for rendering and handling user interaction.

[jordwest]

@nb @aduth Thank you for the reviews and discussion. This is something that confuses me about Redux - it seems to solve a lot of problems by introducing a central store, but on the other hand it's like introducing a major god object 😄

I think there's two different issues here that it might help to look at in different ways:

• Persistent/API Data: For persistent data, the component hierarchy often won't match the data structure. For example, the post editor component tree doesn't match the post object tree (for good reason), which makes it difficult to tell which components are affecting what data. There you have the situation where multiple components at various nesting levels in completely different trees are manipulating an object at various nesting levels and causing issues like the unnecessary re-renders (ref: p4TIVU-11B-p2). I'm not totally familiar with Redux selectors but it seems like they could come in handy here - passing them down to components rather than the raw data itself could help in making data access more granular.
• UI state data In the case of this PR, the data introduced here is purely for UI display purposes so the data hierarchy can be matched exactly with the component hierarchy. For example, the AdvancedOptions component only uses the immediate children of the advancedSettings object [1]. It then passes down the posts, pages, and feedback objects to corresponding OptionFieldset components, which in turn use only the direct children of that data structure. It seems like at this stage it would be overkill to de-normalize this data structure to avoid nesting of the data?

[1] Advanced Options and Settings are actually the same, somehow I ended up naming them two things - I'll change them both to 'Settings'.

[dmsnell]

If I can add one more bit to the conversation, as having played with Redux and datastore for quite a while now, I think it's very critical that we weigh carefully the most pushed-for techniques on the web. It's not that I disagree with Dan Abramov or anything like that, but I've noticed often that there are very few projects on our scale with React, Redux, Flummox, Phlem, Phrogs, and Bogs. The separation between data components and visual components is incredibly important, but we also have to consider the other factors at our scale, one of which can be the proliferation of components.

From what I can gather on the wider Interwebs, most of what is said and discussed concerns the 95% of projects out there where more of the concern is not abusing these frameworks. We should follow 95% of their advice. In the other cases, we should have a clear warrant for not following them.

Two principles that I like to consider are how much data needs to be passed up and down the hierarchy and how significant any particular component will be.

For example, if we end up passing data down seven levels of components we have inseparably tied together all these components making a very fragile system full of data passing code when there's no need. On the other hand, if we have seven levels of components all watching the store we could run into performance issues as everything acts on its own and everything gets tied into the stores.

There's a difference, I think, in something like the exporter. How many instances of this particular component will ever be on display at once? Just a single instance - it's not like a button or a Card. Because of this, I think we can almost eliminate any performance concerns we might have because a single component won't make a dent against the issues something else might bring which on average is there in thirty different places across the app.

Generic settings widgets are great candidates for splitting off into UI-only components because they don't have any intrinsic ties to their data. On that note, they don't even need a data-gathering component because whatever form or module that displays the settings components should already have that data available - pure UI win. On the other hand, aren't specific-setting components already intrinsically tied to their data? Something like a <WhatKindsOfThingsToExportSetting /> should only ever be used by the export by its very nature - not a big violation to get a little messier with the data ties.

The dogma will save us most of the time, but there's very very very little good information on the Internet on guidelines for apps our size (it's a great opportunity for us to learn and share!). Much more, it's significantly more difficult to filter out the noise since there are thousands of articles on Reactifluxiness.

Hope this helps!

[jkudish]

cc @aduth who implemented redux for publicize connections in #338

[aduth]

I've spent my time off today watching Dan Abramov's new Redux tutorial series. It's an excellent set of videos and I strongly recommend watching them all, but the last half-dozen or so videos (20-26) particularly touch on some of the concepts we're debating here around presentational vs. data components. I fully intend to post something more substantial as a reflection of my thoughts on the tutorial, but I thought it worthwhile to add a few snippets to our discussion here:

Another point in favor of separating data components from visual components is that it makes it easier for us in the case that we ever choose to move away from Redux in favor of another framework.

...it decouples your rendering from Redux. So if you later choose to move your project to another framework, such as Relay, you will not have to change all your components because you can keep the presentational components exactly the same. (7:55)

https://egghead.io/lessons/javascript-redux-extracting-presentational-components-addtodo-footer-filterlink

He goes on to say that intermediate container components can avoid the need to have data components from the top of the tree track props which are only used by components further down the tree. Interesting to me that this recommendation is made, as it's been pointed out that the Redux docs themselves encourage data components to be rendered only at the top of the hierarchy (i.e. router).

A counter-point to my view in the videos in favor of @nb's point for simplicity above is that he doesn't always find the need to split container components from presentational components if the component is very simple.

...there isn't really a lot of presentation or behavior here, and it's easier to keep them together until we figure out how to split the presentation. (2:50)

Separating the container and the presentational components is often a good idea, but you shouldn't take it as a dogma. Only do this when it truly reduces the complexity of your code base. (5:50)

https://egghead.io/lessons/javascript-redux-extracting-container-components-visibletodolist-addtodo

In the last few videos covering react-redux's connect function, I learned to have a much better appreciation for the power provided by connect's second argument, mapDispatchToProps. This function can help to avoid the need for declaring a component at all and rather use only the connect function for specifying the data (mapStateToProps) and behavior (mapDispatchToProps) needed by the presentational component. The <AdvancedOptionsSectionContainer /> component in my gist above could therefore be simplified to:

function mapStateToProps( state, ownProps ) {
    const enabled = state.exporter.ui.toJS().advancedSettings[ ownProps.section ].isEnabled;
    return { enabled };
}

function mapDispatchToProps( dispatch, ownProps ) {
    return {
        toggleSection: dispatch( toggleSection( ownProps.section ) )
    };
}

export default connect( mapStateToProps, mapDispatchToProps )( AdvancedOptionsSection );

Open questions for items that I'm not totally sold one way or the other yet:

• Should these connectors still live in independent files, separate from the visual component (i.e. not export a connected function from within the same file as the visual component)? I'm leaning towards saying yes, the containers should be defined in an independent file.
• Should it be dogma that container components and visual components are always separated? Even watching the video series and Dan Abramov's recommendation against being so strict, I'm still leaning towards saying yes, they should always be separate.
• Naming convention for container components. "-Container" as a suffix feels like it does very little to describe the purpose of the container component, though it seems like the purpose may not always be as clear-cut as it is in Dan's video series (i.e. <VisibleTodoList /> wrapping <TodoList /> and limiting todos passed to those matching the current filter).

[gziolo]

Can we extract this reducer into its own file, to keep on style, like we do it in shared/lib/site-settings/reducers.js?

[jordwest]

Hmm, I'm not sure if that would be necessary until this file grows. This reducer is still related to the exporter and there are no further levels of reducer nesting. Here I've basically followed the pattern introduced in /shared/lib/sharing/publicize/reducers.js

[jordwest]

@aduth, @nb: Do you think these architectural decisions are blockers for this PR? I'm hoping to have the exporter ready for testing soon, so maybe I could revisit this after the initial release.

@aduth: I had a watch of Dan's videos over the weekend, they really help to make sense of Redux. I agree with the points you made above. Although on one hand I think naming the data components -Container or even -Data or similar makes it immediately clear that it's not a presentational component, on the other hand it's starting to feel very Java-esque 😄

[dmsnell]

Is this habit adding the combineReducers() call? If we only have a single reducer, it's just adding calls where they don't need to be.

[jordwest]

There's a kind of hidden feature of calling combineReducers that isn't really obvious; It automatically creates a branch on the state tree for that reducer to manage. So you end up with a tree of:

{
  siteSettings: {
    exporter: {
      ui: {
        advancedSettings: { ... }
      }
   }
}

By handing back the ui reducer itself instead of wrapping it in the combineReducers call, the state tree would be:

{
  siteSettings: {
    exporter: {
      advancedSettings: { ... }
   }
}

Of course since ui is currently the only reducer it would make sense to cut out that branch entirely, but in an upcoming PR I'll be adding another branch to manage the non-UI state/the data retrieved from the API, so I've left it in there for now.

[dmsnell]

I'm not sure how hidden this feature is, as it's part of the official documentation on it. There's nothing magic about it either. If you know you will be using this for more subtrees, then it's fine to leave in, but if not then I'd recommend taking it out. It's not too bad to add it in as we go to avoid jumping ahead of ourselves, though this way doesn't look as pretty.

export default ( state, action ) => ( { ui: ui( state, action ) } );

[dmsnell]

hi @jordwest - I gave some feedback. Please let me know what you think!

[jordwest]

Thanks @dmsnell, I've responded in the line comments

[jordwest]

This PR may need to change when #751 is merged

[omarjackman]

This class name should be exporter since your using exporter__export-button and exporter__title below

[jordwest]

Fixed in latest push

[jordwest]

Thanks for the review @omarjackman, I've made the suggested changes

[jordwest]

I've just force-pushed an update in response to the changes introduced by #751. This simply moves /shared/lib/site-settings to /client/store/site-settings, updates relevant imports to the new path, and uses context.store instead of context.reduxStore.

@dmsnell, @omarjackman: since you've already had a look, would you mind going over this again and give it the 👍 or 👎 for a merge? It's still hidden behind a feature-flag and there are a couple more PRs with further work: #1126 and #1359

cc: @dllh

[dmsnell]

Did we not conclude that it is no longer necessary to wrap the component in a function call?

[aduth]

Did we not conclude that it is no longer necessary to wrap the component in a function call?

Correct, if not already, this branch will need to be rebased against master as of #1498, where this will error unless rendered as a direct child element.

[jordwest]

Fixed in latest push

[dmsnell]

@jordwest I left a few quick comments but feel free to merge after you respond to/ignore them 😄