Pure component destructuring #94
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