A React.js case study | Hacker News Reader

A React.js case study

blog.krawaller.se

153 pointsxngzng39 comments

Thanks for putting this together, always interesting to see people use React for the first time! I did a code review of sorts and cleaned up the code to be more idiomatic React. You can find the series of commits with in-depth comments on GitHub [1]. I may even write this up as a blog post on my website [2].

In short:

- Don't pass around components and call methods on them; prefer to pass props instead

- Conditional rendering instead of hiding, exactly as [3]

- Use the JSX harmony transforms

- Declare props

- Use classSet

- setState only needs to include props that change, rest stay the same

[1] https://github.com/ianobermiller/reactexperiment/commits/

[2] http://ianobermiller.com

[3] https://news.ycombinator.com/item?id=8247422

I agree with all of these changes. Code-style-wise, I prefer to call the actual handler implementations `handleFoo` as opposed to `onFoo`. It's a very small detail, but it makes code a little easier to sift through (as well as search/replace), because you always know that `onFoo` is a prop (whose implementation is foreign to the component) and `handleFoo` is a method (whose implementation is native to the component).

Not a bad idea. We would typically use _onClick for any component handler methods, so the _ would distinguish it. We have an internal transform that mangles anything in a module starting with _ so that it cannot be accessed outside.

I also prefix with _, if only to indicate what's private and what's API. Mind sharing your transform?

Wow, thank you so much for this! Reading through your changes I do feel a bit silly. And wiser! :)

Absolutely! It is really hard to write idiomatic code your first time through with a framework. No need to feel silly at all. No way I would have spotted these things if I hadn't been using React every day for the past year or so!

>Use the JSX harmony transforms Thank you for flagging the --harmony flag in the JSX compiler. Just now I was testing Google traceur and es6-module-transpiler and did not realise I already have some of that available for free. I couldn't find any specific information on what subset of ES6 is supported the JSX compiler - would you have a link outlining that?

https://github.com/facebook/jstransform/tree/master/visitors

Thank you!

TLDR: JSX transformer makes the following ES6 features compatible with old browsers (IE8+) * arrow functions [http://tc39wiki.calculist.org/es6/arrow-functions/] * classes [https://github.com/esnext/es6-class] * destructuring [http://fitzgeraldnick.com/weblog/50/] * object concise methods [http://ariya.ofilabs.com/2013/03/es6-and-method-definitions....] * rest parameters [http://tc39wiki.calculist.org/es6/rest-parameters/] * template strings [http://tc39wiki.calculist.org/es6/template-strings/] * object literal property shorthand [http://tc39wiki.calculist.org/es6/object-literal-enhancement...]

Notably absent: default parameter values and block scoping ("let")

Wouldn't one rather use Traceur over JSTransform? It's ES6 coverage seems rather more extensive.

Because if you are doing React and JSX you are already using JSTransform. If it gets you most of what you need, why add another dependency?

True, but if you're using JSX on the server side, the dependency doesn't really cost anything. JSTransform's intended purpose, as far as I can see, is as a general-purpose syntax transformer (the ES6 transforms, of which there is only a handful, come as a bonus), whereas Traceur is a dedicated transpiler that is intended to be a complete implementation of ES6.

> I first tried to do this: [...] ...which actually worked, but generated a React error message about an unmounted component.

I suspect that you were doing something wrong that triggered this error. Unmounting components based on state is perfectly idiomatic React, and I do it all the time:

    {this.state.mode === 'map' && <Map/>}

You pay for the added DOM work, of course, but this is usually negligible in my experience, especially in your case. You do have to make sure you follow the component lifecycle so that you don't try to use getDOMNode() or similar with a component that is not currently mounted.

How does React work under the hood. JavaScript would normally throw an error if it found <Map /> in the source code. How does React do this? Is it JS?

This is not React's, but a javascript preprocessor called JSX that compiles to JS (like LESS -> CSS).

http://facebook.github.io/react/docs/jsx-in-depth.html

Something like <Map foo={bar}/> would compile to a function call Map({foo: bar}). Behind the scenes, React is not dealing with HTML.

You can use React without JSX, but it's useful.

Is React a client-side framework, or server-side?

It's primarily client-side, but React components output a virtual representation of the DOM, so it's relatively simple to render a page on the server and return HTML.

Generally you run the JSX pre-processor server-side and then everything else gets run client-side.

It's a client-side framework. You'll generally run the JSX preprocessor as part of your website's build process (i.e. before uploading it to a server).

both; either

I see some criticism here but I think it is worth pointing out this looks like a person learning to use a new technology and sharing their experience, which is great.

Sometimes that means using non-idiomatic constructs or just having some rough edges. Please be considerate.

I'm not particularly fond of the way React mixes presentation (returning nodes in render()) and logic. Particularly things like:

    var classes = _.reduce(["flipped","correct","wrong"],function(m,c){return m+(this.state[c]?c+" ":"");},"",this);

seem very inelegant compared to say, Angular's ngClass, which does the same thing in a data-driven way. But, that's not to say Angular is free from template logic either.

This isn't a React issue; this is a failure to use an abstraction where appropriate. React offers the classSet plugin for exactly this use case.

http://facebook.github.io/react/docs/class-name-manipulation...

I think the React authors agree that React classes shouldn't contain excessive logic; if your class does, consider extracting it to somewhere more appropriate.

Oh, indeed, I've completely missed the classSet plugin! That would indeed make the code more elegant. And fair point about the logic perhaps being too complicated here.

Thanx for the pointers!

The classSet addon works nicely for the cases I've run into.

http://facebook.github.io/react/docs/class-name-manipulation...

I find that the ability to trivially make the equivalent of Rails "helper" functions as auxiliary methods on the Component class actually tends to encourage separation of logic - it's the same file so there's no context switch needed. On top of that, since JSX makes ternary statements look much more elegant than bracketed if statements (whose conditions can be a great hiding place for complex logic) it further encourages breaking your logic out into auxiliary functions.

Why do

    className={this.state.playing ? "hidden" : "showing"}

instead of

  <div>
      {(this.state.playing
           ? <Board/>
           : <Wordform/> )}
  </div>

It is much simpler to animate between two classes in straight CSS. That's the primary reason I favor the `className` approach.

React has a solution for that which actually automatically applies class names as components are inserted and removed: http://facebook.github.io/react/docs/animation.html. It's not perfect, and it's better than doing it manually.

I touch on this in a post - I did try the latter, but got a React warning in the console about unmounted components. That, together with a comment in the official docs, made me go with the show/hide approach.

Although as pointed out in the Disqus comment, choosingg what to render is also fine, I probably did something silly stemming from not fully understanding the component life cycle.

Performance, I think.

There's now a follow-up to this post, walking through a refactor based on the comments given here and elsewhere. Thank you all for the feedback!

https://news.ycombinator.com/item?id=8273026

Doesn't work properly for me using FF 31.0

I can see the words on the tiles before clicking them.

Might be I've been sloppy with adding -moz- to some css rules. Will take a look!

Same, I can see the words (backwards) before clicking a tile. Chrome works fine, except it seems like after a match the first click on a tile doesn't do anything, and I have to click again to make it flip.

The not doing anything is intentional - only when the pair is resolved and the status row goes back to saying "choose a tile" can you click a new tile. This "feature" really needs to be communicated better though, sorry abouy that.

Auto-prefixer might help...

https://www.npmjs.org/package/grunt-autoprefixer

Ironically, Firefox supports the unprefixed backface-visibility property, but the demo only sets the -webkit- prefixed version.

Off-topic: Why do people use lodash over underscore.js?

Reply on news.ycombinator.com