Appears to wrap other functions than createClass()

[gaearon]

As reported in gaearon/react-transform-catch-errors#25, it seems that the plugin regressed in 2.x and thinks React.shape is something like React.createClass just because it receives an object as the argument.

[gaearon]

I think the problem is here.

Most likely that these two lines should end with || rather than &&:

babel-plugin-react-transform/src/index.js

Line 77 in 63cafc9

!matchesPatterns(path.get('superClass'), this.superClasses) &&

babel-plugin-react-transform/src/index.js

Line 122 in 63cafc9

!matchesPatterns(path.get('callee'), this.factoryMethods) &&

Please feel free to submit a fix, add tests for it, and I’ll be happy to merge.

[nfcampos]

Why is the expected behaviour in this test wrapping the class? Not all classes with a render method are React components
https://github.com/gaearon/babel-plugin-react-transform/blob/master/test/fixtures/code-class-with-render-method/actual.js

To me it looks like that's one of the tests I wanted to add (expecting it not to wrap the class) to make sure #81 and #78 were fixed, no?

[gaearon]

We should expect that

class Foo {
  render() {}
}

does not get wrapped.

Same for

var Foo = PropTypes.shape({ 
  render: PropTypes.func
})

It also should not be wrapped.

[nfcampos]

Exactly, that's why this test can't be right https://github.com/gaearon/babel-plugin-react-transform/blob/master/test/fixtures/code-class-with-render-method/expected.js

Also, none of these should be wrapped either, right?
https://github.com/gaearon/babel-plugin-react-transform/blob/master/test/fixtures/code-call-expression-with-render-method/actual.js

[gaearon]

Exactly, that's why this test can't be right

Yeah. It’s wrong.

Also, none of these should be wrapped either, right?

They shouldn’t. However they should be wrapped if factory is specified as one of the values of the optional factoryMethods array config option.

[nfcampos]

It looks like to me that this line shouldn't even be there

babel-plugin-react-transform/src/index.js

Line 78 in 63cafc9

!isReactLikeClass(path.node)

We never want to wrap a class that doesn't extend React.Component (we might if we wanted to support extending a class that itself extends React.Component but whatever), right? If it doesn't pass that superclass test there's really no reason to check whether it has a render method is there?

[gaearon]

If you change it to be || then I think it makes sense. "If either it's not a descendant -or- it doesn't have a render method, bail out because it is definitely not something we can wrap."

[nfcampos]

the goal:

if (shouldNotWrap)
  return
else
  wrap()

a) current implementation:

if (doesntExtendSuperClass && doesntHaveRenderMethod)
  return
else
  wrap()
// !(doesntExtendSuperClass && doesntHaveRenderMethod) === (extendsSuperClass || hasRenderMethod)

b) change to ||:

if (doesntExtendSuperClass || doesntHaveRenderMethod)
  return
else
  wrap()
// !(doesntExtendSuperClass || doesntHaveRenderMethod) === (extendsSuperClass && hasRenderMethod)
// so this would only wrap descendants of the super class that have a render method
// which makes all the current tests (but one) fail
// but is maybe what we want actually, seeing as the render() method is required in react classes (see https://facebook.github.io/react/docs/component-specs.html#render

c) remove the render method check:

if (doesntExtendSuperClass)
  return
else
  wrap()
// !(doesntExtendSuperClass) === (extendsSuperClass)
// so this would wrap all classes that are descendants of the super class
// which fails only 2 of the current tests (the 2 tests that are expecting the wrong thing)

so which one do we want? (b) or (c)?

[gaearon]

I think we want (b). Only descendants with render methods are good to go.