Framework: Add i18n-translate codemod

[ockham]

This codemod:

• Looks for instances of createClass-created React components that use this.translate (provided by the auto-injected i18n mixin)
• Adds import { localize } from 'i18n-calypso' before the first import it finds. If there's already something else being imported from i18n-calypso, the codemod is smart enough to add the { localize } import to that statement.
• Wraps the createClass instance in localize() (thus providing a translate prop to the component)
• Replaces this.translate with this.props.translate

To test:

• make build
• bin/codemods/i18n-mixin client/my-sites/ads/. Or any directory. Try stuff that doesn't have unit tests (see below).
• (Since this doesn't add whitespace inside localize's parens, do s/localize\((\S*)\)/localize( $1 )/g.)
• Replace \t (one tab, four spaces -- not properly displayed here on GitHub!) with \t\t (two tabs) -- required since that's another thing that codemods tend to mess up.

Limitations:

• There needs to be at least one import statement at the top of each file you want to change. I'd recommend running @jsnmoon's ./bin/codemods/commonjs-imports path-to-transform/ first -- this will convert requires to imports.
• If your component has unit tests, the codemod is likely to break them. The solution is to separately export the unwrapped component and use that from inside the unit test; pass lodash's identity there as translate prop.
• Wrapping components in localize can also break your refs.
• Generally speaking, jscodeshift based codemods tend to screw up some of our whitespace -- in particular, the spaces inside parens (notably for localize).

FYI @jsnmoon @ehg @stephanethomas @gziolo @mtias

[matticbot]

Test live: https://calypso.live/?branch=add/translate-codemod

[matticbot]

@ockham This PR needs a rebase

[gziolo]

I think I found out why Facebook is migrating all projects to use prettier.) It's something that can significantly simplify reviewing changes introduced by codemods :)

We should also revisit split between internal and external dependencies which doesn't play well with automated code refactoring.

I tested it with client folder and it successfully updated 270+ files. It has skipped 3 files for some reasons, it didn't say why.

It has 2 issues to fix:

import i18n, { localize, localize, localize } from 'i18n-calypso';

import { localize } from 'i18n-calypso';
import { localize } from 'i18n-calypso';

We can fix missing or broken whitespaces with eslint --fix on modified files. You can cherry pick only rules you want to apply on selected files.

[gziolo]

By the way. I think it wouldn't be that bad idea to merge it as soon as 2 issues I mentioned are fixed. It can only help people to migrate their code to use localize.

Another idea. Can we have interactive pre-commit hook? We could detect missing localize and ask if it should be added to the PR.

[ockham]

Thanks for the review @gziolo!

It has 2 issues to fix: [...]

Should be fixed now.

We can fix missing or broken whitespaces with eslint --fix on modified files. You can cherry pick only rules you want to apply on selected files.

Last I checked I don't think the whitespace rule that we'd need here was available for the --fix option 😕

Another idea. Can we have interactive pre-commit hook? We could detect missing localize and ask if it should be added to the PR.

We do have an ESLint rule to guard against this.translate -- think that's enough? (If we do want the pre-commit hook, I'd rather do it in a follow-up PR so this one isn't blocked.)

[ockham]

cc @jsnajdr b/c #13712. Maybe we can modify this codemod (or add a separate one) to replace "static" i18n.translate calls by this.props.translate, too.

[jsnajdr]

Instead of wrapping the React.createClass call with localize, maybe the codemod should wrap the export instead:

const Search = React.createClass({ ... });
export default localize( Search );

This pattern makes HOC composition easier, and also doesn't break static methods and properties:

const Search = localize(React.createClass({
  statics: { instances: 0 },
  componentWillMount() { Search.instances++ }, // !!! Search is a different class
});

@ockham What do you think? Would you mind if I pushed a fix to this branch?

[ockham]

@jsnajdr I don't mind, as long as it doesn't break other stuff. As discussed over at #13759 (comment):

Any reason the codemod is wrapping the declaration instead of the line with the export (how we usually do it)?

It was way easier to wrap localize directly around createClass (for which there is a readily available util in react-codemod) than to account for different possible cases such as export default, non-default export, variable assignment, etc.

So we need to keep those other cases in mind, which I think makes things quite a bit harder.

[jsnajdr]

So we need to keep those other cases in mind, which I think makes things quite a bit harder.

I'll check how often there special cases with non-default export actually occur. It would be great if we could wrap the export instead of the createClass call.

[jsnajdr]

I added code that detects the special case where the component is assigned to a variable and then exported as default. In such case, the localize() wrapping is performed in the export statement.

In the remaining cases (and there are plenty of variants), the React.createClass statement is wrapped as before.

There's one more case worth handling:

export default connect(...)(TheComponent);

Codemods are fun!

[jsnajdr]

Added a special case for export default connect() and module.exports = connect() 🚀

[ockham]

Nice! Seems to work well. The code is a bit less easy to grok now, but that's almost inevitable. I was originally thinking we might optimistically wrap the first use of whatever variable createClass would be assigned to, but yours is more fine-grained.

I plan on merging tomorrow so the codemod is available in time for the Calypso Framework Hangout on Thursday.

[jsnajdr]

Next thing I plan to do is to codemod the i18n.translate calls. I'll do it as a separate codemod, as this one is already complex enough. i18n.translate calls will be modified also in ES6 classes, the import statement will be a bit different...

[gziolo]

Great job on this one!

I was wondering if we should pipe codemodes and transform all module.exports to ES6 exports first and then use one codemod to wrap them with localize :)

[ockham]

I was wondering if we should pipe codemodes and transform all module.exports to ES6 exports first and then use one codemod to wrap them with localize :)

I was thinking it'd be preferable to do different transforms in separate steps (and PRs)...

[gziolo]

It has landed 🎉