Setup react, photon-kit and flux architecture

[NickTikhonov]

This PR deals with #8. Planned work:

• integration of React as the front-end/rendering framework
• integration of Photon-kit using React Photon-kit components.
• setting up Flux architecture to completely separate app logic from representation and rendering.

Perhaps we can use something like Redux instead of implementing the architecture from scratch. What do you think @johndbritton @tarebyte ?

Side note: the build is going to break because of a bug in ESLint. A fix has been made but not published.

[NickTikhonov]

Huh, the build did not break - this is odd 😮

[NickTikhonov]

Not sure why travis wasn't getting the error I was getting locally, but running eslint with the --debug flag showed that it was running on .js files in dist - something it shouldn't be doing. I fixed this by adding an ignore file.

[johndbritton]

Perhaps we can use something like Redux instead of implementing the architecture from scratch.

I think Redux is the recommended way to do things, but I don't have a ton of experience. I say go for it if you think it makes sense.

[NickTikhonov]

@johndbritton I think I will - I read some blog posts comparing the different frameworks around, and everybody is praising Redux, even the engineers at Facebook. I'll start with it for now, but I'll take care to make as much of the logic Redux-agnostic as I can so we can change later if needed :)

[NickTikhonov]

Hey @johndbritton @tarebyte - I think this is ready for review 🎉

[tarebyte]

@btmills @atareshawty would either of you mind giving this a 👀 over?

[atareshawty]

@tarebyte I've only had a chance to skim a little. Here's one thing that I noticed though:

When creating a component, the two ways I've experienced/read about are either React.createClass or class ItemList extends React.Component

Referencing (https://github.com/education/classroom-desktop/pull/12/files#diff-e406471fd3bfe87c9712e64367df9253)

Using the later option would look something like this

import React, { Component, PropTypes } from 'react';
import { Pane, ListGroup } from 'react-photonkit';

import Item from './Item';

export default class ItemList extends Component {

  static propTypes = {
    items: PropTypes.array.isRequired,
    handleItemClick: PropTypes.func.isRequired
  }

  render() {
    return (
      <Pane>
        <ListGroup>
          {items.map(({ active, id, text }) =>
            (
              <Item
                active={active}
                handleClick={() => { this.props.handleItemClick(id) }}
                key={id}
                text={text}
              />
            )
        </ListGroup>
      </Pane>
    );
  }
}

Some benefits of this:

• Once you start dealing with state, you're going to want to start using this.setState(state), which you get from extending Component
• You only have to declare your propTypes once. Now, you have them as 'destructured' properties of an object getting passed to your component.
• Personally, I like the syntax although I've read a lot of differing opinions there

I have a small amount of experience, so @btmills would be the person to ask there.

Hope I helped a little. I'm excited to see how this turns out 🚀

[btmills]

Creating functions inside the render method is usually an anti-pattern, as they'll be re-created on every render, putting unnecessary pressure on the garbage collector. (The react/jsx-no-bind lint rule talks a bit about it.)

[btmills]

From an idiomatic React perspective, this is really good code 👍

(@atareshawty - we use ES6 Component on our stuff because, back when we started it, createClass was the only option, and porting from createClass to Component is much easier than the next step, which will be Component to the pure functional components we see here. As much as possible, functional components are the preferred method for green field projects today.)

@NickTikhonov nice separation of container and presentational components. For functional components, I recommend checking out recompose. Specifically, the withHandlers() higher-order component ought to help with #12 (comment). In fact, the docs for that give you a better explanation than I did in my comment.

[NickTikhonov]

Thanks for having a glance over and for your suggestions @atareshawty @btmills :) I'll definitely take a look at recompose today and fix the issue before merging this in.

[NickTikhonov]

@btmills - I enabled react/jsx-no-bind which flagged the line you commented on. I read up the docs, and it seems that the arrow function is the problem here since it's analogous to calling bind on an existing function. The error went away when I replaced the arrow function with:

handleClick={function () { handleItemClick(todo.id) }}

I am guessing the above won't cause any problems on re-render since the VM can cache the unbound function - what do you think? I've taken a look at withHandlers from recompose, and I'm struggling to apply it to this case. Out of interest, could you provide an example of how it can be used here? Thanks for your help! 🍻

[NickTikhonov]

I'm going to merge this in and open up a new Issue/PR for a UI mock in React that implements the design proposed in #11 :)

[btmills]

I am guessing the above won't cause any problems on re-render since the VM can cache the unbound function - what do you think?

@NickTikhonov I can't say this for sure, but I'm guessing it wouldn't be able to cache because it needs to maintain a reference to the todo in the closure. Let me clone this and see what I can find out...

diff --git a/app/components/Item.jsx b/app/components/Item.jsx
index ea07c98..7d85863 100644
--- a/app/components/Item.jsx
+++ b/app/components/Item.jsx
@@ -1,20 +1,27 @@
 import React, { PropTypes } from "react"
 import {ListItem} from "react-photonkit"

+const handlers = window.handlers = new Set()
+
 const Item = ({
   text,
   active,
   handleClick
-}) => (
-  <div
-    onClick={handleClick}>
-    <ListItem
-        image="https://avatars3.githubusercontent.com/u/1744446?v=3&s=400"
-        title={text}
-        subtitle="subtitle"
-        active={active} />
-  </div>
-)
+}) => {
+  handlers.add(handleClick)
+  console.log(handlers.size)
+
+  return (
+    <div
+      onClick={handleClick}>
+      <ListItem
+          image="https://avatars3.githubusercontent.com/u/1744446?v=3&s=400"
+          title={text}
+          subtitle="subtitle"
+          active={active} />
+    </div>
+  )
+}

 Item.propTypes = {
   text: PropTypes.string.isRequired,

The above diff adds code to keep track of the handlers that the Item gets passed. If you run it, you'll see it logging a continuously incrementing size every time the state changes.

diff --git a/app/components/Item.jsx b/app/components/Item.jsx
index ea07c98..ae58951 100644
--- a/app/components/Item.jsx
+++ b/app/components/Item.jsx
@@ -1,25 +1,40 @@
 import React, { PropTypes } from "react"
 import {ListItem} from "react-photonkit"
+import { withHandlers } from "recompose"
+
+const handlers = window.handlers = new Set()
+
+const enhance = withHandlers({
+  handleClick: (props) => () => {
+    props.handleClick(props.id)
+  }
+})

 const Item = ({
   text,
   active,
   handleClick
-}) => (
-  <div
-    onClick={handleClick}>
-    <ListItem
-        image="https://avatars3.githubusercontent.com/u/1744446?v=3&s=400"
-        title={text}
-        subtitle="subtitle"
-        active={active} />
-  </div>
-)
+}) => {
+  handlers.add(handleClick)
+  console.log(handlers.size)
+
+  return (
+    <div
+      onClick={handleClick}>
+      <ListItem
+          image="https://avatars3.githubusercontent.com/u/1744446?v=3&s=400"
+          title={text}
+          subtitle="subtitle"
+          active={active} />
+    </div>
+  )
+}

 Item.propTypes = {
+  id: PropTypes.number.isRequired,
   text: PropTypes.string.isRequired,
   active: PropTypes.bool.isRequired,
   handleClick: PropTypes.func.isRequired
 }

-export default Item
+export default enhance(Item)
diff --git a/app/components/ItemList.jsx b/app/components/ItemList.jsx
index d50880c..66d246a 100644
--- a/app/components/ItemList.jsx
+++ b/app/components/ItemList.jsx
@@ -12,9 +12,10 @@ const ItemList = ({
         return (
           <Item
             key={todo.id}
+            id={todo.id}
             text={todo.text}
             active={todo.active}
-            handleClick={function () { handleItemClick(todo.id) }}
+            handleClick={handleItemClick}
           />
         )
       })}

If, instead, we use withHandlers, you'll see that the size of the set stays constant at 2, no matter how many times the state changes.

Removing the logging code, we end up with this:

diff --git a/app/components/Item.jsx b/app/components/Item.jsx
index ea07c98..248f3a4 100644
--- a/app/components/Item.jsx
+++ b/app/components/Item.jsx
@@ -1,5 +1,14 @@
 import React, { PropTypes } from "react"
 import {ListItem} from "react-photonkit"
+import { withHandlers } from "recompose"
+
+const handlers = window.handlers = new Set()
+
+const enhance = withHandlers({
+  handleClick: (props) => () => {
+    props.handleClick(props.id)
+  }
+})

 const Item = ({
   text,
@@ -17,9 +26,10 @@ const Item = ({
 )

 Item.propTypes = {
+  id: PropTypes.number.isRequired,
   text: PropTypes.string.isRequired,
   active: PropTypes.bool.isRequired,
   handleClick: PropTypes.func.isRequired
 }

-export default Item
+export default enhance(Item)
diff --git a/app/components/ItemList.jsx b/app/components/ItemList.jsx
index d50880c..66d246a 100644
--- a/app/components/ItemList.jsx
+++ b/app/components/ItemList.jsx
@@ -12,9 +12,10 @@ const ItemList = ({
         return (
           <Item
             key={todo.id}
+            id={todo.id}
             text={todo.text}
             active={todo.active}
-            handleClick={function () { handleItemClick(todo.id) }}
+            handleClick={handleItemClick}
           />
         )
       })}

Hope this helps! 🍻

[NickTikhonov]

Awesome, thanks for the example! I think I now understand what is happening 🙌 . I'll be sure to use this pattern in the future.