Hacker News Reader

Comment by wingspan | Hacker News Reader

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

View on HN

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.

Reply on news.ycombinator.com