Codemods: Add assign-component-to-displayname-variable codemod

[spen]

Summary

This codemod aims to fix up instances where a component is directly wrapped by a HoC and exported.
It tackles the same issue that #18390 aims to fix, but retrospectively.

The Problem

Wrapping a .createClass or class extends Component instance in localize is a bit of an ugly pattern and likely encourages folks towards the c( b ( a( Component ) ) ) pattern for enhancing components - either directly or indirectly through some flavour of the following, where enhancements end up scattered:

const SomeComponent = localize( -ComponentDeclaration- );

export default connect( ...)( SomeComponent );

I've seen a couple of instance like the above, but by their nature they're pretty hard to search for.

Also, when we export localize( React.createClass() ); we lose the ability to shallow render the component in unit tests and make access to the base component trickier.

Testing

I've added some test files to show the effects of this codemod. If we decide to proceed with this codemod I'll remove them (or replace with actual 'expected vs actual' type testing.

Run the following to see these effects:

npm run codemod assign-component-to-displayname-variable bin/codemods/test-files-will-be-deleted

Notes:

I've written this codemod with flexibility in mind. Though the focus was to fix cases of localize( class extends... it will pick out components from any arbitrary HoC or function call. Since the component isn't necessarily the first and only argument of such a HoC or function call it also maintains the argument order when substituting the component with a variable name.

[matticbot]

Test live: https://calypso.live/?branch=add/assign-component-to-displayname-variable

[spen]

Throwing errors looks pretty awful so I tried console.log which looks pretty nasty too - is it worth having anything here at all or better just to allow this to fail silently?

[samouri]

Can you give an example of what calleeNameValue represents? It doesn't seem to be like the lack of it should be an error. Usually if a codemod doesn't need to modify a file then it just returns the original file.source unmodified

[spen]

calleeNameValue Would be localize( ... ) or any other wrapping function.
I'd seen errors been thrown in (IIRC) some react codemod. I think it makes most sense to just return early without making any noise, then as you say, return the original / don't call .replaceWith

[samouri]

IMO, error should mean that there is something really wrong. like an issue with the source file, or an unexpected edge case (multiple default exports). Standard noop flow can be silent

[spen]

Yeah I'm feeling that too, I'll squish out those errors/logs and just have it return early :)

[samouri]

@spen: something I've found helpful when working on codemods is to open a second branch that runs the codemod on the entire client folder.

The diff there should show exactly how this codemod works + what its transformation looks like. When I tried running this on client it only modified 3 files, half were unmodified, and the other half errored

[spen]

Thanks for taking a look @samouri - that is weird though, I'm finding that 100+ files are changed... It might just be that I've not force pushed after rebasing (since the i18n codemod was run).

Before that was run this was more a preemptive move :)

[spen]

Also, I've just pushed after a rebase with master - if you have time would you mind trying to run over client once more?
Here's my output:

Results:
0 errors
4831 unmodified
0 skipped
101 ok
Time elapsed: 35.933seconds

[samouri]

nit:

This should always return either 0 or 1 result. We could return early in the case of 0 and not have to write the rest in terms of a loop with:

const defaultExportDeclaration = _.head(
  root.find( j.ExportDefaultDeclaration )
);

if ( ! defaultExportDeclaration ) {
  root.toSource(); // not sure if this line is correct
}

// continue rest without for loop
const calleeNameValue = node.get( 'declaration', 'callee', 'name' ).value;
...

[spen]

I do kind of agree, though a .forEach over a 0 length array would surely be inconsequential, nothing would be run right?
I did originally have this early return in place but figured it'd actually be safer to run in a forEach.
Happy to refactor though :)

[samouri]

i might be wrong, but I think this whole file is currently being indented with spaces instead of tabs.

can we change it to tabs instead? (and if you want autoformatting feel free to prepend the file with: /** @format */

[spen]

Oops! Bit of an oversight on my part! All fixed up now thanks to prettier :)

[samouri]

remove the test files and merge away!