7 ms·

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

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/ https://github.com/ianobermiller/reactexperiment/commits/ [2] http://ianobermiller.com http://ianobermiller.com [3] https://news.ycombinator.com/item?id=8247422 https://news.ycombinator.com/item?id=8247422

krawaller 12y ago
Wow, thank you so much for this! Reading through your changes I do feel a bit silly. And wiser! :)
- wingspan 12y ago
  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!
e1g 12y ago
>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?
- andreypopp 12y ago
  https://github.com/facebook/jstransform/tree/master/visitors https://github.com/facebook/jstransform/tree/master/visitors
  - e1g 12y ago
    Thank you! TLDR: JSX transformer makes the following ES6 features compatible with old browsers (IE8+) * arrow functions [http://tc39wiki.calculist.org/es6/arrow-functions/ http://tc39wiki.calculist.org/es6/arrow-functions/] * classes [https://github.com/esnext/es6-class https://github.com/esnext/es6-class] * destructuring [http://fitzgeraldnick.com/weblog/50/ http://fitzgeraldnick.com/weblog/50/] * object concise methods [http://ariya.ofilabs.com/2013/03/es6-and-method-definitions.html http://ariya.ofilabs.com/2013/03/es6-and-method-definitions....] * rest parameters [http://tc39wiki.calculist.org/es6/rest-parameters/ http://tc39wiki.calculist.org/es6/rest-parameters/] * template strings [http://tc39wiki.calculist.org/es6/template-strings/ http://tc39wiki.calculist.org/es6/template-strings/] * object literal property shorthand [http://tc39wiki.calculist.org/es6/object-literal-enhancements/ http://tc39wiki.calculist.org/es6/object-literal-enhancement...] Notably absent: default parameter values and block scoping ("let")
lobster_johnson 12y ago
Wouldn't one rather use Traceur over JSTransform? It's ES6 coverage seems rather more extensive.
- wingspan 12y ago
  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?
  - lobster_johnson 12y ago
    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.
lobster_johnson 12y ago
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).
- wingspan 12y ago
  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.
  - lobster_johnson 12y ago
    I also prefix with _, if only to indicate what's private and what's API. Mind sharing your transform?

krawaller 12y ago

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

wingspan 12y ago
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!

e1g 12y ago

>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?

andreypopp 12y ago
https://github.com/facebook/jstransform/tree/master/visitors https://github.com/facebook/jstransform/tree/master/visitors
- e1g 12y ago
  Thank you! TLDR: JSX transformer makes the following ES6 features compatible with old browsers (IE8+) * arrow functions [http://tc39wiki.calculist.org/es6/arrow-functions/ http://tc39wiki.calculist.org/es6/arrow-functions/] * classes [https://github.com/esnext/es6-class https://github.com/esnext/es6-class] * destructuring [http://fitzgeraldnick.com/weblog/50/ http://fitzgeraldnick.com/weblog/50/] * object concise methods [http://ariya.ofilabs.com/2013/03/es6-and-method-definitions.html http://ariya.ofilabs.com/2013/03/es6-and-method-definitions....] * rest parameters [http://tc39wiki.calculist.org/es6/rest-parameters/ http://tc39wiki.calculist.org/es6/rest-parameters/] * template strings [http://tc39wiki.calculist.org/es6/template-strings/ http://tc39wiki.calculist.org/es6/template-strings/] * object literal property shorthand [http://tc39wiki.calculist.org/es6/object-literal-enhancements/ http://tc39wiki.calculist.org/es6/object-literal-enhancement...] Notably absent: default parameter values and block scoping ("let")

lobster_johnson 12y ago

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

wingspan 12y ago
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?
- lobster_johnson 12y ago
  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.

lobster_johnson 12y ago

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).

wingspan 12y ago
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.
- lobster_johnson 12y ago
  I also prefix with _, if only to indicate what's private and what's API. Mind sharing your transform?