Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add codemods: React.js #13070

Merged
merged 5 commits into from
Jul 21, 2017
Merged

Add codemods: React.js #13070

merged 5 commits into from
Jul 21, 2017

Conversation

jsnmoon
Copy link
Contributor

@jsnmoon jsnmoon commented Apr 12, 2017

Part of #11688.

This change adds codemods in bin/codemods for porting legacy React.js code.

  • react-create-class: Safely replaces React.createClass with React.Component or createReactClass from 'create-react-class'. (source)
  • react-prop-types: Replaces React.PropTypes with prop-types; applicable for React 15.5+ where React.PropTypes has been extracted to a separate package. (source)

Please note that this currently uses by personal fork with couple of bug fixes on top of react-codemod. The following PRs have been filed upstream:

Todo:

  • Fix class codemod bug erroring out on trailing comma (see this issue)
  • Thoroughly test class codemod on client/
  • Resolve issues with class codemod (Most pressing issue has been resolved
  • Thoroughly test prop-types codemod on client/

Issues:

  • react-create-class codemod
    • Incorrect removes react import for JSX files when falling back to createClass import
      • ex. client/post-editor/media-modal/gallery/preview-shortcode.jsx (react import unnecessarily removed)
      • ex. client/post-editor/editor-ground-control/index.jsx (multiple imports are removed)
      • ex. client/my-sites/themes/thanks-modal.jsx (malformed import after transform)
    • Does not assign a class name if React.createClass was directly exported
      • Noted - not significant enough to block merge
      • ex. client/post-editor/media-modal/gallery/remove-button.jsx
    • Favors CommonJS import notation if the file contains any CommonJS imports
      • Noted - not significant enough to block merge
      • ex. client/post-editor/post-editor.jsx

@jsnmoon jsnmoon self-assigned this Apr 12, 2017
@matticbot matticbot added OSS Citizen [Size] M Medium sized issue labels Apr 12, 2017
@jsnmoon jsnmoon changed the title Add codemods: React.js migration Add codemods: React.js Apr 12, 2017
@jsnmoon jsnmoon changed the base branch from master to add/cjs-to-es2015-modules-transform April 13, 2017 01:30
@jsnmoon jsnmoon force-pushed the add/react-codemods branch from db01e2f to e2c6648 Compare April 13, 2017 01:30
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] M Medium sized issue labels Apr 13, 2017
@jsnmoon jsnmoon force-pushed the add/react-codemods branch from e2c6648 to e9f7b27 Compare April 13, 2017 01:33
@matticbot matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Apr 13, 2017
@jsnmoon jsnmoon force-pushed the add/cjs-to-es2015-modules-transform branch from 1137323 to 040d869 Compare April 17, 2017 23:46
@jsnmoon jsnmoon changed the base branch from add/cjs-to-es2015-modules-transform to master April 19, 2017 21:38
@jsnmoon jsnmoon force-pushed the add/react-codemods branch from fa3640b to 7e7f003 Compare April 19, 2017 21:39
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Apr 19, 2017
@jsnmoon jsnmoon removed the [Size] XL Probably needs to be broken down into multiple smaller issues label Apr 19, 2017
@jsnmoon jsnmoon mentioned this pull request Apr 22, 2017
@ockham
Copy link
Contributor

ockham commented Apr 27, 2017

  • react-create-class: Safely replaces React.createClass with React.Component or createReactClass from 'create-react-class'. (source)

How well does this work? Last time I tried it on the Calypso root dir I think it only changed some 4 components or so. (I'm speculating it might be due to our use of this.translate(), but I'm not sure.)

@jsnmoon
Copy link
Contributor Author

jsnmoon commented May 10, 2017

@ockham: I was under the impression it worked pretty reliably from my initial testing. To verify, I ran the create-class codemod on client/ today and here are the results:

  • Successfully processed: 699 files
    • Many cases of React.createClass were converted into classes extended from React.Component.
    • Many cases of React.createClass were converted into createClass imported from createReactClass package. These usually occur due to use of mixins other than PureRenderMixin or use of deprecated React API calls like getDOMNode, isMounted, replaceProps, replaceState, or setProps.
  • Errored out: 39 files
    • I'm individually investigating these errors, but it'll take me some time to figure out exactly why these are happening - will update in the future.

Here are some issues I discovered from the successfully processed files:

  1. The codemod does not assign a class name if React.createClass was directly exported
    • export default React.createClass( ... ) becomes export default class extends React.Component { ... }
  2. The codemod seems to favor CommonJS import notation if the file contains any CommonJS imports.
  3. If a react import is done with declarator siblings in CommonJS notation, the codemod sometimes erroneously removes the entire import declaration.
    • My hunch is that the codemod doesn't properly handle CommonJS imports with sibling declarators. This issue can be skirted by converting these to ES module imports and then running the react-create-class codemod.

@jsnmoon jsnmoon force-pushed the add/react-codemods branch from 7e7f003 to 185d872 Compare May 10, 2017 04:27
@matticbot matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label May 10, 2017
@@ -0,0 +1,41 @@
#!/usr/bin/env node
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file seems to be missing the executable bit currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed!

package.json Outdated
@@ -184,6 +184,7 @@
"nock": "8.0.0",
"nodemon": "1.4.1",
"react-addons-test-utils": "15.4.0",
"react-codemod": "github:reactjs/react-codemod#d6563d9",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there not a published version of this? Otherwise, github links can be shorter:
reactjs/react-codemod#d6563d9

https://docs.npmjs.com/files/package.json#github-urls

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There doesn't seem to be an npm AFAICS. FWIW, I added the devDep without SHA stamp (thus tracking master) a while back.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gwwar Unfortunately, react-codemod isn't published on npm nor are releases properly tagged on their git repo, so I'll go ahead and shorten this dependency to your suggestion!

@jsnmoon jsnmoon force-pushed the add/react-codemods branch from 89a0a74 to b2aab6e Compare May 24, 2017 19:05
@jsnmoon jsnmoon force-pushed the add/react-codemods branch from b2aab6e to 8d0c017 Compare June 2, 2017 21:49
@matticbot matticbot removed the [Size] XL Probably needs to be broken down into multiple smaller issues label Jun 2, 2017
@matticbot matticbot added [Size] XL Probably needs to be broken down into multiple smaller issues and removed [Size] L Large sized issue labels Jun 12, 2017
@jsnmoon
Copy link
Contributor Author

jsnmoon commented Jun 12, 2017

Rebased off of master and changed the react-codemod dependency to point at my personal fork containing bug fixes relevant to Calypso.

@matticbot
Copy link
Contributor

@jsnmoon This PR needs a rebase

@matticbot
Copy link
Contributor

@jsnmoon This PR needs a rebase

@ockham ockham force-pushed the add/react-codemods branch from 8cebbca to 33c96eb Compare July 16, 2017 11:22
@matticbot matticbot added [Size] L Large sized issue and removed [Size] XL Probably needs to be broken down into multiple smaller issues labels Jul 16, 2017
@ockham
Copy link
Contributor

ockham commented Jul 16, 2017

Rebased, and re-shrunk. This has been lingering for a while, so I think it'd be good to get it in soonish.

Rebased off of master and changed the react-codemod dependency to point at my personal fork containing bug fixes relevant to Calypso.

Is this stuff going to be/has been fixed upstream? If so, it'd be obviously nice to continue to use upstream. Anyway, it'd be cool if you could link to upstream PRs from here so we can keep track.

One more bit of feedback: Since this PR contains a codemod to use the new standalone prop-types npm, I think we should add it to package.json.

@ockham
Copy link
Contributor

ockham commented Jul 16, 2017

One more bit of feedback: Since this PR contains a codemod to use the new standalone prop-types npm, I think we should add it to package.json.

Oh, same for create-react-class.

@jsnmoon
Copy link
Contributor Author

jsnmoon commented Jul 17, 2017

Is this stuff going to be/has been fixed upstream? If so, it'd be obviously nice to continue to use upstream. Anyway, it'd be cool if you could link to upstream PRs from here so we can keep track.

Good point! I have reactjs/react-codemod#122, reactjs/react-codemod#132, reactjs/react-codemod#156, and reactjs/react-codemod#157 open right now. Adding this to the original PR.

Unfortunately, I'm not sure if my PRs will ever land given that they've just been... sitting there in the repo.

@jsnmoon jsnmoon force-pushed the add/react-codemods branch from 33c96eb to 41b634f Compare July 17, 2017 17:24
@jsnmoon
Copy link
Contributor Author

jsnmoon commented Jul 17, 2017

One more bit of feedback: Since this PR contains a codemod to use the new standalone prop-types npm, I think we should add it to package.json.

Oh, same for create-react-class.

Done! create-react-class already existed as a dependency, so I went ahead and updated it to the latest version for good measure.

Rebased and re-wrapped. I'll aim to land this sometime this/next week!

@ockham ockham force-pushed the add/react-codemods branch from 41b634f to 8c187b4 Compare July 21, 2017 16:09
@ockham
Copy link
Contributor

ockham commented Jul 21, 2017

Re-based and re-shrunk one last time. As discussed with @jsnmoon in Slack, I'm going to merge this on his behalf!

@jsnmoon
Copy link
Contributor Author

jsnmoon commented Jul 21, 2017

Thanks so much @ockham! :)

@ockham ockham merged commit 3adac11 into master Jul 21, 2017
@ockham ockham deleted the add/react-codemods branch July 21, 2017 16:23
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jul 21, 2017
@psealock psealock mentioned this pull request Sep 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants