ReactComponent.isValidComponent returns true for component classes

[sophiebits]

The last two assertions in this new test don't pass because of the duck-typing check added in fc2805f.

@sebmarkbage Not quite sure what you had in mind here. Not every object whose .type is a component class is a valid descriptor… right?

it('should identify valid components correctly', function() {
  var Component = React.createClass({
    render: function() {
      return <div />;
    }
  });

  expect(ReactComponent.isValidComponent(<div />)).toEqual(true);
  expect(ReactComponent.isValidComponent(<Component />)).toEqual(true);

  expect(ReactComponent.isValidComponent(null)).toEqual(false);
  expect(ReactComponent.isValidComponent(true)).toEqual(false);
  expect(ReactComponent.isValidComponent({})).toEqual(false);
  expect(ReactComponent.isValidComponent("string")).toEqual(false);
  expect(ReactComponent.isValidComponent(React.DOM.div)).toEqual(false);  // FAIL
  expect(ReactComponent.isValidComponent(Component)).toEqual(false);  // FAIL
});

[sebmarkbage]

This is an intermediate step. Currently the duck typing is flawed regardless. Descriptors will probably get an inheritance chain so that instanceof ReactDescriptor works as a safer check.

Only descriptors should pass. We could probably check for props too. It is expected that component classes themselves fail the test. Therefore it should also be renamed.

On Feb 24, 2014, at 7:42 AM, Ben Alpert notifications@github.com wrote:

The last two assertions in this new test don't pass because of the duck-typing check added in fc2805f.

@sebmarkbage Not quite sure what you had in mind here. Not every object whose .type is a component class is a valid descriptor… right?

it('should identify valid components correctly', function() {
var Component = React.createClass({
render: function() {
return

;
}
});

expect(ReactComponent.isValidComponent(

)).toEqual(true);
expect(ReactComponent.isValidComponent()).toEqual(true);

expect(ReactComponent.isValidComponent(null)).toEqual(false);
expect(ReactComponent.isValidComponent(true)).toEqual(false);
expect(ReactComponent.isValidComponent({})).toEqual(false);
expect(ReactComponent.isValidComponent("string")).toEqual(false);
expect(ReactComponent.isValidComponent(React.DOM.div)).toEqual(false); // FAIL
expect(ReactComponent.isValidComponent(Component)).toEqual(false); // FAIL
});
—
Reply to this email directly or view it on GitHub.

[sophiebits]

Right -- with the current code, isValidComponent returns true when passed a component class, which is wrong.

[sebmarkbage]

That's right. We should add that unit test.

Out of curiosity, how did you find this?

On Feb 24, 2014, at 1:51 PM, Ben Alpert notifications@github.com wrote:

Right -- with the current code, isValidComponent returns true when passed a component class, which is wrong.

—
Reply to this email directly or view it on GitHub.

[sophiebits]

I was adding a warning for passing a component class to renderComponent because of this Stack Overflow question: http://stackoverflow.com/questions/21948048/react-cant-get-past-no-method-mountcomponentintonode.

[sebmarkbage]

I think this is fixed. Not sure if we have a solid unit test to cover this.