Class transform with property initializer #54

Merged
merged 65 commits into from Jul 8, 2016

Projects

None yet

5 participants

@keyanzhang
Member
keyanzhang commented Jun 13, 2016 edited

How it works

  1. Determine if mixins are convertible. We only transform a createClass call to an ES6 class component when:
    • There are no mixins on the class, or
    • options['pure-component'] is true, the mixins property is an array and it only contains pure render mixin (the specific module name can be specified using options['mixin-module-name'], which defaults to react-addons-pure-render-mixin)
  2. Ignore components that:
    • Call deprecated APIs. This is very defensive, if the script finds any identifiers called isMounted, getDOMNode, replaceProps, replaceState or setProps it will skip the component
    • Explicitly call this.getInitialState() and/or this.getDefaultProps() since an ES6 class component will no longer have these methods
    • Use arguments in methods since arrow functions don't have arguments. Also please notice that arguments should be very carefully used and it's generally better to switch to spread (...args) instead
    • Have inconvertible getInitialState(). Specifically if you have variable declarations like var props = ... and the right hand side is not this.props then we can't inline the state initialization in the constructor due to variable shadowing issues
    • Have non-primitive right hand side values (like foo: getStuff()) in the class spec
  3. Transform it to an ES6 class component
    1. Replace var A = React.createClass(spec) with class A extends React.Component {spec}. If a component uses pure render mixin and passes the mixins test (as described above), it will extend React.PureComponent instead
      • Remove the require/import statement that imports pure render mixin when it's no longer being referenced
    2. Pull out all statics defined on statics plus the few special cased statics like childContextTypes, contextTypes, displayName, getDefaultProps(), and propTypes and transform them to static properties (static propTypes = {...};)
      • If getDefaultProps() is simple (i.e. it only contains a return statement that returns an object) it will be converted to a simple assignment (static defaultProps = {...};). Otherwise an IIFE (immediately-invoked function expression) will be created (static defaultProps = function() { ... }();). Note that this means that the function will be executed only a single time per app-lifetime. In practice this hasn't caused any issues — getDefaultProps should not contain any side-effects
    3. Transform getInitialState()
      • If there's no getInitialState() or the getInitialState() function is simple (i.e., it only contains a return statement that returns an object) then we don't need a constructor; state will be lifted to a property initializer (state = {...};)
        • However, if the object contains references to this other than this.props and/or this.context, we can't be sure about what you'll need from this. We need to ensure that our property initializers' evaluation order is safe, so we defer state's initialization by moving it all the way down until all other property initializers have been initialized
      • If getInitialState() is not simple, we create a constructor and convert getInitialState() to an assignment to this.state
        • constructor always have props as the first parameter
        • We only put context as the second parameter when (one of) the following things happen in getInitialState():
          • It accesses this.context, or
          • There's a direct method call this.x(), or
          • this is referenced alone
        • Rewrite accesses to this.props to props and accesses to this.context to context since the values will be passed as constructor arguments
          • Remove simple variable declarations like var props = this.props; and var context = this.context
        • Rewrite top-level return statements (return {...};) to this.state = {...}
          • Add return; after the assignment when the return statement is part of a control flow statement (not a direct child of getInitialState()'s body) and not in an inner function declaration
    4. Transform all non-lifecycle methods and fields to class property initializers (like onClick = () => {};). All your Flow annotations will be preserved
      • It's actually not necessary to transform all methods to arrow functions (i.e., to bind them), but this behavior is the same as createClass() and we can make sure that we won't accidentally break stuff
  4. Generate Flow annotations from propTypes and put it on the class (this only happens when there's /* @flow */ in your code and options['flow'] is true)
    • Flow actually understands propTypes in createClass calls but not ES6 class components. Here the transformation logic is identical to how Flow treats propTypes
    • Notice that Flow treats an optional propType as non-nullable
      • For example, foo: React.PropTypes.number is valid when you pass {}, {foo: null}, or {foo: undefined} as props at runtime. However, when Flow infers type from a createClass call, only {} and {foo: undefined} are valid; {foo: null} is not. Thus the equivalent type annotation in Flow is actually {foo?: number}. The question mark on the left hand side indicates {} and {foo: undefined} are fine, but when foo is present it must be a number
    • For propTypes fields that can't be recognized by Flow, $FlowFixMe will be used

A few steps left:

  • no --no-super-class option
    • Fix readme
  • Support react-addons-pure-render-mixin
  • Fix flow annotations (the return type of arrow functions is missing for now)
  • Remove var ReactComponentWithPureRenderMixin = require('ReactComponentWithPureRenderMixin') when transformation happens
    • Check the scope before removing it (for cases where there are multiple components in a single file)

Update: discussed with @spicyj in person and here are the changes/required steps to finish before shipping this:

  • Transform propTypes to Flow annotations (props: ...) (a7f71a3)
    • We want to keep both propTypes and flow annotations so we only make it stricter
    • also transforms properties like count: (12: number), to count: number = 12; (231f629)
  • Support primitives as properties
  • Do not sort properties/methods so that the diff stays clean
  • Transform every method except lifecycle methods to arrow functions to make sure we don't break anything
  • Drop context and/or props in constructor(props, context) whenever possible
  • Add and fix no-use-before-define linter rule

New issues:

  • Fix early returns in getInitialState (b8bb77a)
  • Call super with props when getInitialState uses only this.props or this.props.foo but has no other usage of this (3936f55)

Good to fix:

  • Call super with props and context when getInitialState uses this in any other way (3936f55)
  • Defer state property initializer evaluation when getInitialState has any references to this (4f64cc2)
  • Bail out for dynamic mixins (ada3fb8)
  • Bail out if user calls this.getInitialState() anywhere (e00677f)
  • Bail out if arguments is used inside React methods (59e3671)
  • Fix compile error with const props = this.props in getInitialState (d67b6a8)
  • displayName should not show up twice
  • Handle nullable prop types properly (595bb97)

Backlog:

  • Add support for higher order components (const X = wrap(React.createClass(..)) crashes codemod) (19b611a)
  • Introduce a lint rule after this to prevent from over-binding
@keyanzhang keyanzhang referenced this pull request in reactjs/core-notes Jun 13, 2016
Merged

Add notes for June, 9 #19

@keyanzhang
Member

Updated to change the existing class codemod instead of creating a new one.

@nathanmarks
nathanmarks commented Jun 14, 2016 edited

@keyanzhang Is that the best idea? The benefit I see to keeping the existing class codemod is that it will better support es6 users who aren't using babel (or babel + stage 1). Class properties and initializers are a stage 1 proposal for es7.

@nathanmarks

More food for thought -- if you want to combine them, could the stage-1 items such as property initializers be optional?

@keyanzhang
Member

@nathanmarks Yeah, I think we could keep the both or them. Updating the existing one makes it much easier to review the diff, but I can switch to create new files later if necessary.

@spicyj
Member
spicyj commented Jun 14, 2016

For us, I think it makes the most sense to keep the version we're planning to use at Facebook in this repo. If people want the older version without property initializers it's easy to check out an old version – but I don't anticipate that many people will want this codemod script and convert all of the components until property initializers are more stable.

@keyanzhang
Member

@gaearon: I added a temporary shrinkwrap file to pin recast to my fork and the tests pass locally. As soon as benjamn/recast#289 gets merged it will disappear 😃

@gaearon
Member
gaearon commented Jun 15, 2016

👍 I’ll review later today, thank you.

@gaearon
Member
gaearon commented Jun 15, 2016

Do we need the --no-super-class option?

No, not anymore. Component base class is required in 15.0+.

Remove var ReactComponentWithPureRenderMixin = require('ReactComponentWithPureRenderMixin') when transformation happens

I think we might want to also support removing import PureRenderMixin from 'react-addons-pure-render-mixin' and var PureRenderMixin = require('react-addons-pure-render-mixin');.

@gaearon gaearon commented on an outdated diff Jun 15, 2016
transforms/class.js
@@ -105,11 +108,14 @@ module.exports = (file, api, options) => {
return !invalidProperties.length;
};
- const hasMixins = classPath => {
- if (ReactUtils.hasMixins(classPath)) {
+ const hasInconvertibleMixins = classPath => {
+ if (
+ ReactUtils.hasMixins(classPath) &&
+ !ReactUtils.hasSpecificMixins(classPath, ['ReactComponentWithPureRenderMixin'])
@gaearon
gaearon Jun 15, 2016 Member

For open source, it’s important that we support PureRenderMixin here. This is how we refer to it in the docs.

@gaearon gaearon commented on an outdated diff Jun 15, 2016
transforms/__testfixtures__/class-test2.output.js
+ <div>{this.state.counter}</div>
+ );
+ }
+}
+
+// Comment
+module.exports = class extends React.Component {
+ static propTypes = {
+ foo: React.PropTypes.bool,
+ };
+
+ static defaultProps = {
+ foo: 12,
+ };
+
+ state = function() { // non-simple
@gaearon
gaearon Jun 15, 2016 Member

This doesn’t look very good. I think most people will be confused by this, and will copy and paste this pattern even when it is unnecessary without understanding what it means.

Let’s embed non-trivial state initializers into the constructor.

@gaearon gaearon commented on an outdated diff Jun 16, 2016
called directly. It checks for `this.foo` but also traces variable
assignments like `var self = this; self.foo`. It does not bind functions
- from the React API and ignores functions that are being called directly
- (unless it is both called directly and passed around to somewhere else)
- * Creates a constructor if necessary. This is necessary if either
- `getInitialState` exists in the `React.createClass` spec OR if functions
- need to be bound to the instance.
- * When `--no-super-class` is passed it only optionally extends
+ from the React API (lifecycle methods) and ignores functions that are being
+ called directly (unless it is both called directly and passed around to
+ somewhere else).
+ * TODO When `--no-super-class` is passed it only optionally extends
@gaearon
gaearon Jun 16, 2016 Member

Don’t forget to remove this TODO 😉

@gaearon
Member
gaearon commented Jun 21, 2016

Please correct me if I’m wrong but I think this would be broken:

import React from 'react';

const App = React.createClass({
  getInitialState() {
    const state = {
      whatever: this.context.whatever
    }
    return state
  },
  render() {
    return <div />
  }
})
App.contextTypes = {
  whatever: React.PropTypes.object
}

It becomes

import React from 'react';

class App extends React.Component {
  constructor() {
    super();
    const state = {
      whatever: this.context.whatever
    }
    this.state = state;
  }

  render() {
    return <div />
  }
}

App.contextTypes = {
  whatever: React.PropTypes.object
}

but this.context is not available in the constructor because we haven’t passed it to super, so this.context.whatever would throw.

@gaearon
Member
gaearon commented Jun 21, 2016

Here is another example that I think would be broken by this:

import React from 'react';

const App = React.createClass({
  getInitialState() {
    const state = this.calculateState()
    return state
  },
  calculateState() {
    return { color: this.props.color }
  },
  render() {
    return <div />
  }
})

It becomes

import React from 'react';

class App extends React.Component {
  constructor() {
    super();
    const state = this.calculateState()
    this.state = state;
  }

  calculateState = () => {
    return { color: this.props.color }
  };

  render() {
    return <div />
  }
}

but this.props is not yet available by the time calculateState is called.

It seems like if getInitialState exists and its body contains references to this, it is unsafe to just call super() and you have to call super(props, context) instead.

@keyanzhang
Member
keyanzhang commented Jun 21, 2016 edited

Ahhhhh, I didn't know that you can assign static properties outside of createClass (like App.contextTypes = ...)!

Yeah you are right. Actually I talked to @spicyj yesterday about this -- I'll change the logic to search for this.context in getInitialState instead.

To quote Ben, "It's only strictly necessary if this.context is accessed in the constructor, but it seems sensible to do it whenever contextTypes is defined because we assign .context from outside regardless of what you pass to super: https://github.com/facebook/react/blob/master/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js#L246 right after instantiation."

But since it's possible to have an assignment after creating a class I think it makes more sense to just scan getInitialState.

@gaearon
Member
gaearon commented Jun 21, 2016

I'll change the logic to to search for this.context in getInitialState instead.

getInitialState may call another function which may use this.context. So I think if getInitialState uses this (e.g. to call that function), it is already unsafe.

@spicyj
Member
spicyj commented Jun 21, 2016

Also didn't know that would work but I guess I'm not surprised. :)

I don't want to add context to the super call except when needed because most people don't know about context and shouldn't have to think about it. If we're concerned about this case let's only add the context arg if "this" is referenced alone or in a direct method call this.x() -- but this.props or this.props.foo() should not cause a context arg to be added.

You could also grep to see how often that pattern comes up. Guessing we only have a dozen or two cases of ".contextTypes =" and maybe all are React.Component. So probably also okay to punt on it.

@gaearon
Member
gaearon commented Jun 21, 2016

I don't want to add context to the super call except when needed because most people don't know about context and shouldn't have to think about it.

I agree but most components would not have constructors anyway. They are only generated when you’re doing something non-trivial in getInitialState.

If we're concerned about this case let's only add the context arg if "this" is referenced alone or in a direct method call this.x() -- but this.props or this.props.foo() should not cause a context arg to be added.

This sounds good. 👍

@spicyj
Member
spicyj commented Jun 21, 2016

this.calculateState(this.props) is mildly common so that you can call it in WillReceiveProps and update your state the same way.

@gaearon
Member
gaearon commented Jun 21, 2016

You could also grep to see how often that pattern comes up. Guessing we only have a dozen or two cases of ".contextTypes =" and maybe all are React.Component. So probably also okay to punt on it.

Fair enough. It’s not just about the context though—we have the same potential issue with props (#54 (comment)). I guess always emitting super(props) could be a good solution if you’re up for it.

@spicyj
Member
spicyj commented Jun 21, 2016

Yeah, I'm fine with writing super(props); whenever we're a little unsure or even just for all components regardless.

@gaearon
Member
gaearon commented Jun 21, 2016

Should we add getChildContext to AUTOBIND_IGNORE_KEYS?

@keyanzhang
Member
keyanzhang commented Jun 21, 2016 edited

Should we add getChildContext to AUTOBIND_IGNORE_KEYS?

Good catch. I'll add a test for it.

@gaearon
Member
gaearon commented Jun 21, 2016

Not sure if this is a pattern anybody uses but the use of arguments seems unsafe to transform:

class Stuff {
  doSomething(a, b, c) {
    console.log(arguments);
  }
}

let x = new Stuff()
x.doSomething(1, 2, 3) // {0: 1, 1: 2, 2: 3}
class Stuff {
  doSomething = (a, b, c) => {
    console.log(arguments);
  }
}

let x = new Stuff()
x.doSomething(1, 2, 3) // {}
@gaearon
Member
gaearon commented Jun 21, 2016

This looks broken. Input:

'use strict';

var React = require('react')

var Loader = React.createClass({
  getInitialState() {
    if (this.props.stuff) {
      return { x: 1 }
    }

    switch (this.props.wow) {
      case 1:
        return this.props.lol ?
          { x: 2 } :
          this.whatever(this.props)
    }

    return this.lol()
  },

  render() {
    return null;
  }
});

module.exports = Loader;

Output:

'use strict';

var React = require('react')

class Loader extends React.Component {
  constructor(props) {
    super(props);
    if (props.stuff) {
      return { x: 1 }
    }

    switch (props.wow) {
      case 1:
        return props.lol ?
          { x: 2 } :
          this.whatever(props)
    }

    this.state = this.lol();
  }

  render() {
    return null;
  }
}

module.exports = Loader;

As you can see, all return statements inside if or switch branches are not transformed, but they should be—they should become assignments to this.state with an immediate return.

@gaearon
Member
gaearon commented Jun 22, 2016

Here is an issue with duplicate declaration:

'use strict';

var React = require('react')

var Loader = React.createClass({
  getInitialState() {
    const props = this.props
    return { x: props.x }
  },

  render() {
    return null;
  }
});

module.exports = Loader;

It becomes:

'use strict';

var React = require('react')

class Loader extends React.Component {
  constructor(props) {
    super(props);
    const props = props
    this.state = { x: props.x };
  }

  render() {
    return null;
  }
}

module.exports = Loader;

In this case const props = props is a compile error.

@gaearon
Member
gaearon commented Jun 22, 2016

This looks incorrect, we should bail out if mixins is anything other than array literal.

Input:

'use strict';

var React = require('react');

const mixins = [ClassNameMixin, PureRenderMixin]

var Loader = React.createClass({
  mixins: mixins,

  render() {
    return <div />;
  }
});

module.exports = Loader;

Output:

'use strict';

var React = require('react');

// oops!
const mixins = [ClassNameMixin, PureRenderMixin]

class Loader extends React.Component {
  render() {
    return <div />;
  }
}

module.exports = Loader;
@gaearon
Member
gaearon commented Jun 22, 2016

We should probably bail out if user calls this.getInitialState() anywhere in the component.
(Yes, apparently this pattern is a thing.)

Input:

'use strict';

var React = require('react');

var Loader = React.createClass({
  getInitialState() {
    return { x: 42 }
  },

  reset() {
    this.setState(this.getInitialState())
  },

  render() {
    return <div onClick={this.reset} />;
  }
});

module.exports = Loader;

Output (will crash!)

'use strict';

var React = require('react');

class Loader extends React.Component {
  state = { x: 42 };

  reset = () => {
    this.setState(this.getInitialState())
  };

  render() {
    return <div onClick={this.reset} />;
  }
}

module.exports = Loader;
@gaearon
Member
gaearon commented Jun 22, 2016

I think we should bail from lifting state if it references this at all—not just this.props.
Here is another example that seems broken now.

Input:

'use strict';

var React = require('react');

var Loader = React.createClass({
  something: 42,

  getInitialState() {
    return { x: this.something }
  },

  render() {
    return <div onClick={this.reset} />;
  }
});

module.exports = Loader;

Output (I think it would be broken because something hasn’t been assigned yet):

'use strict';

var React = require('react');

class Loader extends React.Component {
  state = { x: this.something };
  something = 42;

  render() {
    return <div onClick={this.reset} />;
  }
}

module.exports = Loader;
@gaearon
Member
gaearon commented Jun 22, 2016 edited

To sum up the todos I found so far.

These need to be fixed:

  • Need to fix early returns in getInitialState (#54 (comment))
  • Need to call super with props when getInitialState uses only this.props or this.props.foo but has no other usage of this (#54 (comment))

These are less severe so we can maybe live with them, but it would be better to fix (at least some of) them:

  • Consider calling super with props and context when getInitialState uses this in any other way (#54 (comment))
  • Consider always creating a constructor if getInitialState has any references to this (#54 (comment))
    • Note: Instead of always creating a constructor I chose the defer the state property initialization if it contains more than just this.props and/or this.context -- Keyan
  • Consider bailing out for dynamic mixins (#54 (comment))
  • Consider bailing out if user calls this.getInitialState() anywhere (#54 (comment))
  • Consider bailing out if arguments is used inside React methods (#54 (comment))
  • Consider fixing compile error with const props = this.props in getInitialState (#54 (comment))
  • Consider adding support for higher order components (const X = wrap(React.createClass(..)) crashes codemod)

Note that almost all of them can lead to potential crashes even though patterns themselves may not be super common.

@keyanzhang
Member
keyanzhang commented Jun 22, 2016 edited

Need to fix early returns in getInitialState

Yeah, I just changed the way we inline initialState (0cfad0a). It works now but I'm not happy with the implementation -- will improve it later.

Consider fixing compile error with const props = this.props in getInitialState

What do you think is a good way to fix this? I can think of 3 approaches -- rename the constructor's parameters when necessary (make them hygienic); make state an IIFE (state = function() { ... }()) which actually solves a lot of problems but the code doesn't look good; or just bail out.

Consider bailing out for dynamic mixins

Fixed in ada3fb8.

I think we should bail from lifting state if it references this at all—not just this.props. Here is another example that seems broken now.

Thanks for catching this. I'll say I'm down with bailing out here -- an alternative approach is to move state all the way down until all the other property initializers have been initialized. It's doable but uh it doesn't feel clean. 3936f55 improved the constructor and lifting logic, but I think it makes sense to make it more strict.

@gaearon
Member
gaearon commented Jun 22, 2016

I can think of 3 approaches -- rename the constructor's parameters when necessary (make them hygienic); make state an IIFE (state = function() { ... }()) which actually solves a lot of problems but the code doesn't look good; or just bail out.

If the right hand side is just this.props, I think you can safely remove the assignment. Otherwise, bail out completely for the class.

I'll say I'm down with bailing out here -- an alternative approach is to move state all the way down until all the other property initializers have been initialized.

I think this might be reasonable. Having non-whitelisted and non-function properties is a rare case anyway so moving state below such properties should solve such cases.

@gaearon gaearon and 1 other commented on an outdated diff Jun 22, 2016
transforms/class.js
)
- );
- }
-
- return statement;
- });
+ ));
+
+ if ( // FIXME is there a better way to check this?
@gaearon
gaearon Jun 22, 2016 Member

What does this do / why is this necessary?

@keyanzhang
keyanzhang Jun 22, 2016 Member

This is to check if the original return is part of a control flow statement (if, switch, etc.) -- if yes then we need to add a return; after the assignment.

@keyanzhang
keyanzhang Jun 22, 2016 Member

The current implementation assumes that people don't call createClass inside a big if statement. 😨 Also it doesn't handle stuff like try catch or promise properly. I think a better way is to get the closestScope of the return and compare it with the scope that getInitialState creates -- do you know if there's a way to compare 2 scopes?

@gaearon
gaearon Jun 22, 2016 Member

Do you ever want to omit return in any case other than when it is direct child of method body?

@keyanzhang
keyanzhang Jun 23, 2016 Member

Yeah, that sounds good.

@keyanzhang keyanzhang commented on an outdated diff Jun 23, 2016
transforms/__testfixtures__/class-flow1.output.js
+
+class Component extends React.Component {
+ props: {
+ optionalArray?: Array<any>,
+ optionalBool?: boolean,
+ optionalFunc?: Function,
+ optionalNumber?: number,
+ optionalObject?: Object,
+ optionalString?: string,
+ optionalNode?: any,
+ optionalElement?: any,
+ optionalMessage?: Message,
+ optionalEnum?: 'News' | 'Photos' | 1 | true | null,
+ optionalUnion?: string | number | Message,
+ optionalArrayOf?: Array<number>,
+ optionalObjectOf?: {[key: string]: number,},
@keyanzhang
keyanzhang Jun 23, 2016 Member

Will remove the trailing comma later: see benjamn/recast#291 (comment)

@keyanzhang keyanzhang changed the title from WIP: Class transform with property initializer to Class transform with property initializer Jun 24, 2016
@keyanzhang
Member

This PR is ready for review. This is quite a big diff so I added some new comments in class-initial-state.input.js to explain the test cases.

Seems like Travis has trouble understanding npm-shrinkwrap.json (it's pointing to my own fork of recast now) but the tests pass locally. Feel free to ping me if there's anything unclear. Thanks @gaearon! cc @spicyj

@jide
jide commented Jun 30, 2016

Shouldn't constructor() and super() be also passed context as argument ?

@gaearon
Member
gaearon commented Jun 30, 2016

It doesn’t have to be passed unless you’re reading this.context in the constructor. React will set those fields automatically. So we opt for not surfacing context whenever possible. See also discussion above on this.

@jide
jide commented Jun 30, 2016

Ok, thanks for the explanation !

Keyan Zhang added some commits Jun 30, 2016
Keyan Zhang use FlowFixMe for unrecognizable types 953386a
Keyan Zhang retain getInitialState()'s return type when possible 3e62530
Keyan Zhang stricter checking of referencing APIs that will be removed 3f700c1
Keyan Zhang optional !== nullable 51494ac
Keyan Zhang support void (undefined) in PropTypes.oneOf() ab1e1a7
Keyan Zhang annotate state type when it's inlined in the constructor 62978f4
Keyan Zhang added 'remove-runtime-proptypes' option ee36b61
Keyan Zhang changed inexplicit any to FlowFixMe 1011552
Keyan Zhang Redesigned the way we start modding components.
Now we search and grab `React.createClass` _call expressions_ directly. The only time that we can't simply replace a `createClass` call path with a new class is when the parent of that is a variable declaration:

`var Foo = React.createClass({...});`
needs to be replaced entirely with
`class Foo extends React.Component {...}`

Now we delay checking it and only when we need to replace a path we take a look at `path.parentPath.value.type` to see if it's a variable declarator. With this commit we should be able to mod any kind of anonymous `createClass` calls no matter how they are defined.
847de6d
Keyan Zhang fixed default and rest params b65424c
@keyanzhang keyanzhang merged commit 8e370b9 into reactjs:master Jul 8, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon
Member
gaearon commented Jul 8, 2016

Congratulations!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment