Adding a codemod for transforming createElement function calls to JSX

[elicwhite]

Transforms

var a = React.createElement( Foo, null, React.createElement("div", { foo: "bar" }), React.createElement( "span", null, "blah" ) ); 

to

var a = <Foo><div foo="bar"></div><span>blah</span></Foo>; 

This codemod is useful for people who start out without using JSX and want to convert their codebase to JSX later.

[elicwhite]

Following alphabetical order

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[cpojer]

Can you add these two cases:

• React.createElement(Foo, {foo: 'bar', ...someObject}) to <Foo foo="bar" {...someObject}> – where the order of properties should be preserved, of course.
• Convert children-less components to <Foo /> instead of <Foo></Foo>

[elicwhite]

Is it correct that

var a = React.createElement(Foo, null); var b = React.createElement("div", null); 

should convert to:

var a = <Foo />; var b = <div></div>; 

Or should div be a self closing element too?

[cpojer]

I think they should both convert to a single tag that closes, like <div />.

[cpojer]

This is awesome! I left a couple of inline comments and I'll set up eslint for this repo.

Two more minor things:

• Can you copy-paste the way printOptions works in the pure-render-transform: https://github.com/reactjs/react-codemod/blob/master/transforms/pure-render-mixin.js#L19 – I need to turn this pattern into some utility function or something, this is really annoying to duplicate :(
• If a transform doesn't touch a file it should return null instead of root.toSource() to avoid unwanted mutations. The other transforms in this repo all count how many things they transform and only return something if they find callers they want to mutate.

[elicwhite]

• Create test for the spread operator
• Return null if file wasn't modified

[cpojer]

oh yeah, can you also flatten your commits into a single one?

[cpojer]

you can just inline this replaceWith below:

root.find(...).replaceWith(...).size()

[elicwhite]

As in combining this and the stuff on line 76?

EDIT replaceWith will automatically iterate over the items? No need for the forEach?

[cpojer]

yeah, you don't need the call to j, replaceWith is part of every collection so you can just call it directly instead of forEach.

[elicwhite]

Neat.

[cpojer]

I like no-parens around single-argument-identifiers in arrows but I'm definitely not going to fight you over that :P @zpao will love you, actually, so this is just about who you wanna score points with.

[elicwhite]

If single-argument-identifiers was enforced one way or the other by eslint I'd be happy to follow it. :)

[cpojer]

rm this empty line please :)

[cpojer]

Alright this looks really solid. I left a couple of inline comments but take your pick on what you actually wanna update – these are mostly just suggestions.

The only thing I'd like to see is to copy this: https://github.com/reactjs/react-codemod/blob/master/transforms/class.js#L500 to make sure it only runs on code that actually has React in scope and doesn't have a React = require('underscore') or something in it :)

[elicwhite]

I'll fix the blank line, and ignore the destructuring.

Can you provide a test case that would fail without that check on React in scope?

[cpojer]

It should be something like doing var React = require('foo'), like you map React to something that is not actually React.

[cpojer]

I also promise to consolidate all that setup code some time, it sucks to copy-paste it.

[elicwhite]

Currently failing for the case of

var React = require('foo'); React.createElement(Foo, 'la'); 

Expected: '' toEqual: 'var React = require('foo'); React.createElement(Foo, 'la');' 

Should my test expect that to return the original contents untransformed, or an empty string as it is now?

Also, I don't see any tests for any of the other transforms that test the behavior of options['explicit-require'] so I don't have examples to check for this.

[cpojer]

yeah it is ok not to test it. If the transform returns null it means that it won't apply any transformations, so you should test for an empty string (and maybe add the coercion from null to an empty string in jest/env.js.

[cpojer]

yay!