Framework: Run i18n-mixin-to-localize codemod #18590

ockham · 2017-10-05T19:00:24Z

TODO:

Look for prototype.translate in tests, remove
Fix mixins ("headless" components still using this.translate)
- sites-list notices: see SitesList Notices: Turn into actual React Component #18625
- form-base: Turn into component, have consumers inherit?
  - Looks okay, all consumers are localize()d now, so form-base should be able to just use this.props.translate().
Check refs. Do we need localize to hoist them?
- Looks okay, see Framework: Run i18n-mixin-to-localize codemod #18590 (comment) and convo below
Grep for files containing this.props.translate but not i18n-calypso. Any takers?

psealock · 2017-10-06T09:35:35Z

Revert PostEditor changes

@ockham Do you know why these tests were failing by chance? It seemed a reference to the Component from ReactDOM.render() was working sometimes, but not others.

ockham · 2017-10-06T09:39:35Z

@ockham Do you know why these tests were failing by chance? It seemed a reference to the Component from ReactDOM.render() was working sometimes, but not others.

I think there are two errors in play here -- 6dc6a62 was just to fix tests; turns out the default export from that file was already being localize; the codemod wrapped the unlocalized component in a localize call, which broken tests

ockham · 2017-10-06T09:43:41Z

The other error I'm currently seeing with PostEditor is this:

The immediate reason for this is that sections.js (as generated by server/bundler/loader) tries to access the isRetry static method from LoadingError (in client/layout/error), but that component is now wrapped in a localize. To mitigate that, one option would be to have localize hoist statics, much like react-redux' connect() does. Or maybe we can rewrite server/bundler/loader and/or client/layout/error to decouple the isRetry logic from the component.

This still doesn't fix the underlying error with the editor, but maybe we'll get more meaningful error messages to track it down once we fix the LoadingError issue.

ockham · 2017-10-06T09:54:27Z

Fix in c17bc44. I think I'll spin up a separate PR for that.

ockham · 2017-10-06T10:11:31Z

So the actual error in PostEditor is

and I think the reason are once again static methods --

wp-calypso/client/components/tinymce/plugins/wpcom-view/views/contact-form/index.jsx

Line 20 in 6e86844

match( content ) {

in this case. After the codemod is run, that component is wrapped in localize. So we might have to modify i18n-calypso to hoist static methods after all.

ockham · 2017-10-06T10:17:30Z

Oh, we might work around that using a similar pattern as https://github.com/Automattic/wp-calypso/blob/master/client/components/tinymce/plugins/wpcom-view/views/simple-payments/index.jsx

psealock · 2017-10-09T00:53:27Z

Oh, we might work around that using a similar pattern as https://github.com/Automattic/wp-calypso/blob/master/client/components/tinymce/plugins/wpcom-view/views/simple-payments/index.jsx

I like this pattern better because it signals intention better than independently exported functions. However, fixing the root cause of the issue by addressing the localize function itself is the best solution.

sirreal · 2017-10-09T11:27:25Z

Grep for files containing this.props.translate but not i18n-calypso. Any takers?

$ ack -L 'i18n-calypso' $(ack --js -l 'this\.props\.(translate|moment|numberFormat)' client)

client/my-sites/checkout/cart/cart-messages-mixin.jsx
client/my-sites/sharing/connections/services/eventbrite.js
client/my-sites/sharing/connections/services/google-photos.js
client/my-sites/sharing/connections/services/instagram.js

I'll have a look at these… cracks knuckles

sirreal · 2017-10-09T11:52:19Z

After some grepping, all the usage seems fine. The cart-messages-mixin could have some extra protection, but that's probably not necessary. See #18651 for more details.

sirreal · 2017-10-09T12:43:47Z

Check refs. Do we need localize to hoist them?

What's the concern here? Here are some findings that may be relevant:

refs in components that need codemodding:

$ ack -l '\Wref=' $(ack --js -l 'this\.(translate|moment|numberFormat)' client )

client/blocks/author-selector/index.jsx
client/components/plans/premium-popover/index.jsx
client/components/tinymce/plugins/contact-form/dialog/field-remove-button.jsx
client/components/tinymce/plugins/wplink/dialog.jsx
client/me/profile-links/add-buttons.jsx
client/me/security-2fa-backup-codes-list/index.jsx
client/me/security-2fa-sms-settings/index.jsx
client/me/security-account-recovery/edit-email.jsx
client/my-sites/domains/components/form/country-select.jsx
client/my-sites/exporter/export-card/post-type-options.jsx
client/my-sites/importer/uploading-pane.jsx
client/my-sites/media-library/header.jsx
client/my-sites/people/team-list/index.jsx
client/my-sites/people/viewers-list/index.jsx
client/my-sites/plugins/plugin-sections/index.jsx
client/my-sites/plugins/plugin-site-update-indicator/index.jsx
client/my-sites/post-selector/selector.jsx
client/my-sites/sharing/buttons/preview-buttons.jsx
client/my-sites/stats/post-trends/day.jsx
client/my-sites/stats/post-trends/index.jsx
client/post-editor/editor-status-label/index.jsx
client/post-editor/editor-sticky/index.jsx
client/post-editor/media-modal/gallery-help.jsx
client/post-editor/post-editor.jsx

There appear to be 84 uses of ref in components with localize. Here's an example:

wp-calypso/client/blocks/app-banner/index.jsx

Lines 160 to 163 in ce892aa

    
           <Card 
        
           	className={ classNames( 'app-banner', 'is-compact', currentSection ) } 
        
           	ref={ this.preventNotificationsClose } 
        
           >

Is verifying that that example works fine enough?

sirreal · 2017-10-10T07:54:48Z

Good thinking!

EDIT: I improved the original to catch a few more cases:

$ ack -l --js '(?<!this)\.refs' client

client/components/drop-zone/test/index.jsx
client/components/root-child/test/index.jsx
client/components/segmented-control/index.jsx
client/components/select-dropdown/index.jsx
client/devdocs/test/doc.jsx
client/extensions/wp-super-cache/components/advanced/directly-cached-files.jsx
client/my-sites/domains/domain-management/list/test/index.js
client/my-sites/themes/themes-magic-search-card/index.jsx

Looks like a manageable list, I'll dig further…

sirreal · 2017-10-10T08:53:58Z

After looking a bit, cleaning up all those refs looks like it might get a bit hairy. Note the especially fun ones like this.refs[ 'item-' + newIndex ].refs.itemLink.

@ockham Let's hear what you think, but it may be best to add the ref hoisting to localize. Sounds like it should really be there anyways (at least until string refs move from "leagacy" to "deprecated").

Here's the full output of non-this refs:

$ ack --js '(?<!this)\.refs' client

client/components/drop-zone/test/index.jsx
60:             expect( tree.refs.zone.parentNode.id ).to.equal( 'container' );
72:             expect( tree.refs.zone.parentNode.id ).to.not.equal( 'container' );
73:             expect( tree.refs.zone.parentNode.parentNode ).to.eql( document.body );

client/components/root-child/test/index.jsx
55:                     expect( tree.refs.parentChild.parentNode.className ).to.equal( 'parent' );
57:                     expect( tree.refs.rootChild.parentNode.parentNode ).to.eql( document.body );
68:                     expect( tree.refs.rootChild.parentNode.className ).to.equal( 'wrapper' );
70:                     expect( tree.refs.rootChild.parentNode.parentNode.parentNode ).to.eql( document.body );

client/components/segmented-control/index.jsx
221:            ReactDom.findDOMNode( this.refs[ 'item-' + newIndex ].refs.itemLink ).focus();

client/components/select-dropdown/index.jsx
171:                                                    self.refs.dropdownContainer.focus();
401:            ReactDom.findDOMNode( this.refs[ 'item-' + newIndex ].refs.itemLink ).focus();

client/devdocs/test/doc.jsx
36:                             const html = ReactDom.findDOMNode( renderedSingleDoc.refs.body ).innerHTML;

client/extensions/wp-super-cache/components/advanced/directly-cached-files.jsx
27:             const newDirectPage = this.refs.newDirectPage.refs.textField;

client/my-sites/domains/domain-management/list/test/index.js
122:                            assert( ReactDom.findDOMNode( component.refs.cancelChangePrimaryButton ) );
126:                            const button = ReactDom.findDOMNode( component.refs.cancelChangePrimaryButton );

client/my-sites/themes/themes-magic-search-card/index.jsx
102:            const { value, selectionStart } = this.refs[ 'url-search' ].refs.searchInput;
142:                            cursorPosition: val.slice( 0, this.refs[ 'url-search' ].refs.searchInput.selectionStart )

ockham · 2017-10-10T10:17:45Z

Oh wow, thanks a lot for the grepping, really helps to see the bigger picture here!

I took a random sample from that list: client/components/root-child/test/index.jsx really only uses the ref for testing, and the tested component was already localize()d before (might be the case for a few other ones, too). I might take some more samples, but I'm kinda adamant to cross fingers and discover the problematic ones in testing. I'm a bit hesitant to add ref hoisting to i18n-calypso when string refs are on their way out...

ockham · 2017-10-10T10:23:40Z

Other instances are textbook cases of ref function attributes, e.g. client/components/segmented-control/index.jsx...

Edit: Okay, maybe a bit trickier than the textbook example -- but on the bright side, it doesn't need to be localize()d!

ockham · 2017-10-10T10:34:04Z

Here's the full output of non-this refs: [...]

I had a look at the rest of the list. Tests should be fine by definition (they wouldn't pass otherwise), and the other ones seem to either use localize() already, or aren't using translate() at all.

ockham · 2017-10-10T10:59:46Z

I just vetted form-base, see note in PR desc. Seems good. Thus: 368d5c0

ockham · 2017-10-10T11:06:26Z

Is it me or does this thing feel way faster now that the auto-injected mixin is gone? 😮

rachelmcr · 2017-10-10T12:26:32Z

I ran through our full e2e tests on this branch and everything looks good. 👍

@jorgefilipecosta

Fixes a bug I introduced with #18590, as reported by @jorgefilipecosta in Slack: > If open media and select an image and then press edit I get an error related with localize it may be related with the last changes Turns out that `EditorMediaModalDetail` is [properly `localize()`d](https://github.com/Automattic/wp-calypso/blob/e7e4265d9b65eccb771c3977b54bc0d272bc3c7b/client/post-editor/media-modal/detail/index.jsx#L100), but `my-sites/media/main` was importing the unlocalized version of the component.

samouri · 2017-10-10T18:23:10Z

Nice work @ockham!!! This was a tricky one

spen · 2017-10-10T19:16:21Z

client/blocks/author-selector/index.jsx


-module.exports = React.createClass( {
+module.exports = localize(React.createClass( {


Related: #18710 will soon be ready for review (once a desc is written).
It aims to fix up instances similar to this ^

It currently only looks at export defaults but could be tweaked to consider module.exports = too :)

ockham added Framework [Status] In Progress labels Oct 5, 2017

ockham self-assigned this Oct 5, 2017

ockham requested review from jsnmoon and samouri October 5, 2017 19:00

ockham force-pushed the codemod/i18n branch from 2654202 to 38b7974 Compare October 6, 2017 09:12

ockham mentioned this pull request Oct 6, 2017

Try: i18-mixin codemod #18604

Closed

ockham mentioned this pull request Oct 6, 2017

Framework: Localize and fix client/layout/error #18610

Merged

This was referenced Oct 6, 2017

TinyMCE: ContactForm View: Localize and fix #18615

Merged

Docs: Replace this.translate() with this.props.translate() #18623

Merged

SitesList Notices: Turn into actual React Component #18625

Merged

ockham force-pushed the codemod/i18n branch from 6c9627b to 4f24f61 Compare October 7, 2017 10:45

psealock mentioned this pull request Oct 9, 2017

My-Sites: SharingButtonsLabelEditor: i18n codemod fix #18646

Merged

sirreal self-requested a review October 9, 2017 07:53

ockham force-pushed the codemod/i18n branch 3 times, most recently from 2043b8b to 6afdc24 Compare October 9, 2017 10:11

ockham mentioned this pull request Oct 9, 2017

PostEditor: Use this.props.moment #18649

Merged

sirreal mentioned this pull request Oct 9, 2017

Cart: Turn cart-messages-mixin into component #18651

Merged

9 tasks

ockham added 5 commits October 10, 2017 12:09

Framework: Run i18n-mixin-to-localize codemod

7f4df8a

Fix PublicizeConnection test

86d97e9

Fix EditorDiscussion test

23bc4aa

DomainSuggestion: Remove translate stub from test

aee4f9e

DomainWarnings: Remove translate stub from test

168e2f9

ockham force-pushed the codemod/i18n branch from 2588a36 to 168e2f9 Compare October 10, 2017 10:09

ockham added 2 commits October 10, 2017 12:56

FormBase: Use this.props.translate

600daf1

Boot: Remove i18n mixin auto-injection

368d5c0

ockham added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Oct 10, 2017

sirreal mentioned this pull request Oct 10, 2017

Courses: Refactor to follow convention #18690

Merged

ockham merged commit dc99dd9 into master Oct 10, 2017

ockham deleted the codemod/i18n branch October 10, 2017 14:07

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 10, 2017

ockham mentioned this pull request Oct 10, 2017

Media: Correctly import EditorMediaModalDetail #18705

Merged

jsnmoon mentioned this pull request Oct 10, 2017

Upgrade to React 15.6 #18555

Merged

spen reviewed Oct 10, 2017

View reviewed changes

ockham mentioned this pull request Oct 11, 2017

InputChrono: Fix devdocs example #18765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Framework: Run i18n-mixin-to-localize codemod #18590

Framework: Run i18n-mixin-to-localize codemod #18590

ockham commented Oct 5, 2017 •

edited

Loading

psealock commented Oct 6, 2017

ockham commented Oct 6, 2017

ockham commented Oct 6, 2017

ockham commented Oct 6, 2017 •

edited

Loading

ockham commented Oct 6, 2017

ockham commented Oct 6, 2017

psealock commented Oct 9, 2017

sirreal commented Oct 9, 2017 •

edited

Loading

sirreal commented Oct 9, 2017

sirreal commented Oct 9, 2017

sirreal commented Oct 10, 2017 •

edited

Loading

sirreal commented Oct 10, 2017

ockham commented Oct 10, 2017

ockham commented Oct 10, 2017 •

edited

Loading

ockham commented Oct 10, 2017

ockham commented Oct 10, 2017

ockham commented Oct 10, 2017

rachelmcr commented Oct 10, 2017

samouri commented Oct 10, 2017

spen Oct 10, 2017


		module.exports = React.createClass( {
		module.exports = localize(React.createClass( {

Framework: Run i18n-mixin-to-localize codemod #18590

Framework: Run i18n-mixin-to-localize codemod #18590

Conversation

ockham commented Oct 5, 2017 • edited Loading

psealock commented Oct 6, 2017

ockham commented Oct 6, 2017

ockham commented Oct 6, 2017

ockham commented Oct 6, 2017 • edited Loading

ockham commented Oct 6, 2017

ockham commented Oct 6, 2017

psealock commented Oct 9, 2017

sirreal commented Oct 9, 2017 • edited Loading

sirreal commented Oct 9, 2017

sirreal commented Oct 9, 2017

sirreal commented Oct 10, 2017 • edited Loading

sirreal commented Oct 10, 2017

ockham commented Oct 10, 2017

ockham commented Oct 10, 2017 • edited Loading

ockham commented Oct 10, 2017

ockham commented Oct 10, 2017

ockham commented Oct 10, 2017

rachelmcr commented Oct 10, 2017

samouri commented Oct 10, 2017

spen Oct 10, 2017

Choose a reason for hiding this comment

ockham commented Oct 5, 2017 •

edited

Loading

ockham commented Oct 6, 2017 •

edited

Loading

sirreal commented Oct 9, 2017 •

edited

Loading

sirreal commented Oct 10, 2017 •

edited

Loading

ockham commented Oct 10, 2017 •

edited

Loading