Reader: Use localize() instead of i18n mixin #13759

ockham · 2017-05-08T16:11:21Z

Let's continue the tradition of using the Reader as a guinea pig for trying out new codemods 😁

Branch made by running the codemod from #13597, and s/localize$(\S*)$/localize( $1 )/g.

To test:

Verify that the Reader is still properly localized.

matticbot · 2017-05-08T16:11:24Z

Test live: https://calypso.live/?branch=update/reader-drop-this-translate

samouri · 2017-05-08T16:25:08Z

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

Current:

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

Preferred:

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

gziolo · 2017-05-08T16:28:34Z

I was about to ask about the same :) It might break one of the Babel's transformations which gives the name to the components. Another thing is that React.createClass is something that Eslint no longer loves :)

samouri · 2017-05-08T16:34:56Z

Let's continue the tradition of using the Reader as a guinea pig for trying out new codemods 😁

Love it btw 🎉

ockham · 2017-05-08T17:56:41Z

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.

…ranslate

ockham · 2017-05-09T21:59:41Z

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

Thanks to @jsnajdr's contribution to #13597, the codemod is now smarter! I re-ran it on client/reader and updated this PR accordingly.

gziolo · 2017-05-10T06:40:26Z

This makes linter very unhappy. Errors from the first container:


/home/ubuntu/wp-calypso/client/reader/update-notice/index.jsx
  39:2  error  Mixed spaces and tabs                           no-mixed-spaces-and-tabs
  42:1  error  Line 42 exceeds the maximum line length of 140  max-len

/home/ubuntu/wp-calypso/client/reader/following-edit/sort-controls.jsx
  33:2  error  Mixed spaces and tabs  no-mixed-spaces-and-tabs

/home/ubuntu/wp-calypso/client/reader/recommendations/posts/index.jsx
  18:3  error  Unexpected var, use let or const instead  no-var
  22:2  error  Mixed spaces and tabs                     no-mixed-spaces-and-tabs

/home/ubuntu/wp-calypso/client/reader/stream/empty.jsx
  40:2  error  Mixed spaces and tabs  no-mixed-spaces-and-tabs

/home/ubuntu/wp-calypso/client/reader/following-edit/import-button.jsx
  72:2  error  Mixed spaces and tabs  no-mixed-spaces-and-tabs

✖ 7 problems (7 errors, 0 warnings)

There are 3 more container to check, but it could be easily fixed by prettier (see #12260) :)

Expect me to advertise prettier more each time we introduce new codemod :D

jsnajdr · 2017-05-10T06:57:25Z

This makes linter very unhappy. Errors from the first container:

Some eslint --fix processing will be necessary here.

when formatting the output, recast doesn't support the space-in-parens rule (or maybe it's just not turned on?)
it has a bug when formatting JSX -- inserts spaces before the opening tag despite the useTabs option

ockham · 2017-05-10T11:44:44Z

I ran ./node_modules/.bin/eslint --fix client/reader/, and it doesn't seem to be able to fix no-mixed-spaces-and-tabs (yet -- I might need to check the Changelog and maybe update to a more recent version).

ockham · 2017-05-10T11:50:26Z

@gziolo Anyway, it looks like most of these lints aren't even introduced by the codemod but seem to pre-exist, no? (Check the affected lines.) If that's the case, I'd say it's fine to ignore them and consider them an orthogonal issue.

ockham · 2017-05-10T11:51:56Z

Hmm, on second glance, I might be wrong about that 😳

ockham · 2017-05-10T11:53:43Z

I ran ./node_modules/.bin/eslint --fix client/reader/, and it doesn't seem to be able to fix no-mixed-spaces-and-tabs (yet -- I might need to check the Changelog and maybe update to a more recent version).

Computer says no: eslint/eslint#6873

ockham · 2017-05-10T11:59:48Z

Resorted to using Atom to replace with . Which seems to have worked 😄

gziolo · 2017-05-10T12:25:09Z

IDE is your friend. That works, too :)
I think you can skip other errors, because they were existing before.

I was using --fix with https://github.com/Automattic/delphin which has a different setup. To be honest I don't remember which rules worked :) I know that some of them work. What I remember is, I decided to explicitly provide rule to be fixed.

gziolo · 2017-05-10T12:31:36Z

client/reader/liked-stream/main.jsx

@@ -3,6 +3,8 @@
 */
 import React from 'react';



I'm wondering why it adds empty line here and in a couple of other places?

I think that's benjamn/recast#371

jsnajdr · 2017-05-10T12:32:01Z

I ran ./node_modules/.bin/eslint --fix client/reader/, and it doesn't seem to be able to fix no-mixed-spaces-and-tabs

If you mean the spaces added before an opening JSX element, I isolated the bug in recast and will be coming up with a patch. It won't be a problem for long, hopefully.

gziolo · 2017-05-10T12:32:41Z

client/reader/site-stream/featured.jsx

@@ -19,7 +20,7 @@ import {
 } from 'reader/stats';
 import cssSafeUrl from 'lib/css-safe-url';

-export default React.createClass( {
+export default localize(React.createClass({


Missing spaces inside localize.

gziolo · 2017-05-10T12:34:13Z

I had two minor comments, otherwise it looks neat. I think it needs to be tested by someone from Reader team and we are good to 🚢

ockham · 2017-05-10T12:57:24Z

I ran ./node_modules/.bin/eslint --fix client/reader/, and it doesn't seem to be able to fix no-mixed-spaces-and-tabs

If you mean the spaces added before an opening JSX element, I isolated the bug in recast and will be coming up with a patch. It won't be a problem for long, hopefully.

Nice! Wanna add a link to the PR for reference? 😄

ockham · 2017-05-10T14:20:36Z

I think it needs to be tested by someone from Reader team and we are good to 🚢

@blowery @bluefuton @samouri pls? 😄

jsnajdr · 2017-05-10T14:47:37Z

Nice! Wanna add a link to the PR for reference?

Here it is: benjamn/recast#404

samouri · 2017-05-10T14:52:28Z

I'll give this a go 🤞

bluefuton · 2017-05-10T14:53:24Z

I've just fixed up the last couple of lint errors 👍

samouri · 2017-05-10T15:49:23Z

I'm on the rest of the style errors ugh
edit: since when were we erroring for things like createClass? I think we should merge this as-is and deal with these unrelated errors separately...

samouri

I say 🚢

ockham · 2017-05-10T16:27:02Z

Thanks for merging @samouri!

ockham added [Feature] Reader The reader site on Calypso. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 8, 2017

ockham self-assigned this May 8, 2017

ockham requested review from blowery, bluefuton and samouri May 8, 2017 16:11

matticbot added the [Size] XL Probably needs to be broken down into multiple smaller issues label May 8, 2017

ockham mentioned this pull request May 8, 2017

React 15.6 migration #13321

Closed

ockham mentioned this pull request May 9, 2017

Framework: Add i18n-translate codemod #13597

Merged

ockham added 2 commits May 9, 2017 23:58

Reader: Use localize() instead of i18n mixin

e14766d

FollowingEditEmptyContent: Use this.props.translate instead of i18n.t…

ba918ac

…ranslate

ockham force-pushed the update/reader-drop-this-translate branch from fbcae70 to ba918ac Compare May 9, 2017 21:58

Reader: Fix whitespace in localize()

c22203a

gziolo mentioned this pull request May 10, 2017

Framework: Use Prettier? #12260

Closed

ockham added 2 commits May 10, 2017 14:12

Fix mixed spaces and tabs introduced by the codemod

9423ff0

Reader: One more lint fix

2cb5418

gziolo reviewed May 10, 2017

View reviewed changes

gziolo approved these changes May 10, 2017

View reviewed changes

ockham added 2 commits May 10, 2017 14:53

Reader: Whitespace fix in FeedFeatured

971e778

Reader: Remove spurious newlines before imports

2ffea10

Fix last couple of lint errors

1be2a0b

samouri approved these changes May 10, 2017

View reviewed changes

samouri added [Status] Ready to Merge and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels May 10, 2017

samouri merged commit a347672 into master May 10, 2017

samouri deleted the update/reader-drop-this-translate branch May 10, 2017 16:26

matticbot removed the [Status] Ready to Merge label May 10, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader: Use localize() instead of i18n mixin #13759

Reader: Use localize() instead of i18n mixin #13759

ockham commented May 8, 2017 •

edited

Loading

matticbot commented May 8, 2017

samouri commented May 8, 2017 •

edited

Loading

gziolo commented May 8, 2017 •

edited

Loading

samouri commented May 8, 2017

ockham commented May 8, 2017

ockham commented May 9, 2017

gziolo commented May 10, 2017

jsnajdr commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

gziolo commented May 10, 2017 •

edited

Loading

gziolo May 10, 2017 •

edited

Loading

ockham May 10, 2017

jsnajdr commented May 10, 2017

gziolo May 10, 2017

gziolo commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

jsnajdr commented May 10, 2017 •

edited

Loading

samouri commented May 10, 2017

bluefuton commented May 10, 2017

samouri commented May 10, 2017 •

edited

Loading

samouri left a comment

ockham commented May 10, 2017

Reader: Use localize() instead of i18n mixin #13759

Reader: Use localize() instead of i18n mixin #13759

Conversation

ockham commented May 8, 2017 • edited Loading

matticbot commented May 8, 2017

samouri commented May 8, 2017 • edited Loading

gziolo commented May 8, 2017 • edited Loading

samouri commented May 8, 2017

ockham commented May 8, 2017

ockham commented May 9, 2017

gziolo commented May 10, 2017

jsnajdr commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

gziolo commented May 10, 2017 • edited Loading

gziolo May 10, 2017 • edited Loading

Choose a reason for hiding this comment

ockham May 10, 2017

Choose a reason for hiding this comment

jsnajdr commented May 10, 2017

gziolo May 10, 2017

Choose a reason for hiding this comment

gziolo commented May 10, 2017

ockham commented May 10, 2017

ockham commented May 10, 2017

jsnajdr commented May 10, 2017 • edited Loading

samouri commented May 10, 2017

bluefuton commented May 10, 2017

samouri commented May 10, 2017 • edited Loading

samouri left a comment

Choose a reason for hiding this comment

ockham commented May 10, 2017

ockham commented May 8, 2017 •

edited

Loading

samouri commented May 8, 2017 •

edited

Loading

gziolo commented May 8, 2017 •

edited

Loading

gziolo commented May 10, 2017 •

edited

Loading

gziolo May 10, 2017 •

edited

Loading

jsnajdr commented May 10, 2017 •

edited

Loading

samouri commented May 10, 2017 •

edited

Loading