Conversation
Needs to be tested Only works if there are no references to `this.props` by itself Would be nice if it would work for (replace) in-render destructuring
If `bar` is assigned to `this.props.bar` we need to remove that, as it's available as a destrctured arg
|
@keyanzhang is the man to review this. |
|
Will do as soon as I find a stable internet connection!
…--
Keyan Zhang
On January 3, 2017 at 11:15:05 AM, Christoph Pojer ***@***.******@***.***)) wrote:
@keyanzhang(https://github.com/keyanzhang) is the man to review this.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub(#94 (comment)), or mute the thread(https://github.com/notifications/unsubscribe-auth/ACKdJFeLWi92N8yAwgAPgVHF9sG2RP2sks5rOnQJgaJpZM4LYlcw).
|
transforms/pure-component.js
Outdated
| const ReactUtils = require('./utils/ReactUtils')(j); | ||
|
|
||
| const useArrows = options.useArrows || false; | ||
| const destructureEnabled = options.destructure || false; |
There was a problem hiding this comment.
Could you rename this option to destructuring? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment
transforms/pure-component.js
Outdated
| const build = ({ functionType }) => (name, body, typeAnnotation, destructure) => { | ||
| const identifier = j.identifier(name); | ||
| const propsIdentifier = buildIdentifierWithTypeAnnotation('props', typeAnnotation); | ||
| const propsArg = [(destructure && destructureProps(j(body))) || propsIdentifier]; |
There was a problem hiding this comment.
Here it drops existing type annotations built by buildIdentifierWithTypeAnnotation.
Input:
class PureWithTypes extends React.Component {
props: { foo: string };
render() {
return <div className={this.props.foo} />;
}
}Output:
function PureWithTypes(
{
foo,
},
) {
return <div className={foo} />;
}| const propNames = new Set(); | ||
| toDestructure.replaceWith(path => { | ||
| const propName = path.value.property.name; | ||
| propNames.add(propName); |
There was a problem hiding this comment.
While this approach (adding names to the set while replacing) is efficient, it introduces some potential shadowing issues. For example:
Input:
class Shadowing extends React.Component {
render() {
let style = { color: 'black' };
return <div style={{...style, ...this.props.style}} />;
}
}Output:
function Shadowing(
{
style,
},
) {
let style = { color: 'black' };
return <div style={{...style, ...style}} />;
}I think a better way here is to scan the whole function twice (using forEach), once for identifiers and once for prop property accesses; then we can compare the two sets to see if it's possible to apply destructuring here.
transforms/pure-component.js
Outdated
| ), | ||
| ] | ||
| ); | ||
| const build = ({ functionType }) => (name, body, typeAnnotation, destructure) => { |
There was a problem hiding this comment.
nit: unnecessary to use a named parameter here
|
hey. Just a note as I already passed my codemod: I would have liked to use this option, but also why not using an arrow function instead of replacing the class by a real function? |
|
@slorber if you use both the |
|
so bad I already did the migration, the useArrow option was not documented :) |
|
I've put in those fixes @keyanzhang |
|
@keyanzhang :ping: |
|
Sorry for the late response -- somehow I missed the email notification. Thanks for the great work @andy-j-d! |
5 participants
Adds a
destructuredestructuringoption topure-componentthat will destructure the props where it's safe to do so.See the tests for use cases:
Input
Output