Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add component stack to invalid element type warning #8495

Merged
merged 4 commits into from
Jan 9, 2017

Conversation

n3tr
Copy link
Contributor

@n3tr n3tr commented Dec 4, 2016

To resolve #7856 and #7859 seems no longer active

Add stack addendum to Invalid element type warning

  • It will get stack from constructed element if the element has _source (file and line number), the stack will be look like In Unknown (at file.js:12).
  • In case jsx babel transform isn't being used, we won't have location info (in element._source). we will use parent's generated stack instead.

Sample Code

let Foo = {};
class Button extends React.Component {
  render() {
    return (
      <div>
        <Foo /> // Foo is invalid element
      </div>
    );
  }
}

class DangerButton extends React.Component {
  render() {
    return <Button />;
  }
}

var ExampleApplication = React.createClass({
  render: function() {
    return (
      <div>
        <DangerButton />
      </div>
    )
  }
});

ReactDOM.render(
  <ExampleApplication />,
  document.getElementById('container')
);

Output (with babel transform)

react.js:3683 Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components). Check the render method of `Button`.
    in Unknown (at example.js:7)
    in Button (at example.js:15)
    in DangerButton (at example.js:23)
    in div (at example.js:22)
    in ExampleApplication (at example.js:30)

Output (without _source from babel)

react.js:3683 Warning: React.createElement: type should not be null, undefined, boolean, or number. It should be a string (for DOM elements) or a ReactClass (for composite components). Check the render method of `Button`.
    in Button (created by DangerButton)
    in DangerButton (created by ExampleApplication)
    in div (created by ExampleApplication)
    in ExampleApplication

We drop the first line which is in Unknown (created by Button) since it's not very useful.

Copy link
Collaborator

@sophiebits sophiebits left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for sending this in! This looks pretty good to me. You'll need to rebase over #8612 if I land it soon.

I actually changed my mind a little bit on the stack – I think the "in Unknown" will be a little weird and I'm also a little uneasy about forcing the tree hook to understand invalid elements. Instead, can we change the "Check the render method of…" to be "Check your code at file.js:12." when a source is available for the element being created and then just use the stack for parent components?

'Warning: React.createElement: type should not be null, undefined, ' +
'boolean, or number. It should be a string (for DOM elements) or a ' +
'ReactClass (for composite components).'
);
// The format is ' in Unknown (at ReactElementValidator-test.js:539)'
expectDev(lines[1]).toMatch(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use normalizeCodeLocInfo like the other tests?

@n3tr
Copy link
Contributor Author

n3tr commented Dec 24, 2016

Wouldn't it be better to show "Check the render method of … (at file.js:12)" if we know the owner and show "Check your code at file.js:12." if we don't. It looks more useful to me in case I have many components in single file. What do you think?

@sophiebits
Copy link
Collaborator

I think it's not necessary because the name will already be in the stack below, and in the case of "render callbacks" the current owner will often not be the "correct" component that is responsible.

@n3tr
Copy link
Contributor Author

n3tr commented Dec 25, 2016

Rebased and updated the code.

Output:

warning.js:36 Warning: React.createElement: type is invalid -- expected a string (for built-in components) or a class/function (for composite components) but got: boolean. Check your code at Button.js:8.
    in Button (at DangerButton.js:6)
    in DangerButton (at App.js:10)
    in div (at App.js:9)
    in App (at index.js:7)

Updated: Remove ( .. ) from addendum

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

I think it's easy to miss the actual place where error happens and go to the parent:

screen shot 2017-01-09 at 15 19 37

My eyes stop at index.js:7 here. I didn't even notice Check your code at here.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

Proposal: break it out with a double newline.

screen shot 2017-01-09 at 15 23 26

We could do this for all warnings since people commonly miss this tip at the very end.

@gaearon
Copy link
Collaborator

gaearon commented Jan 9, 2017

I’ll get this in for now and look at delimiting with newlines later for all warnings together.
Thank you so much!

@gaearon gaearon merged commit fb7e494 into facebook:master Jan 9, 2017
@gaearon gaearon added this to the 15-hipri milestone Jan 9, 2017
bvaughn pushed a commit that referenced this pull request Mar 29, 2017
* Show Source Error Addemden if __source available

* Add Parent Stack on invalid element type

* refactor to use normalizeCodeLocInfo

* Remove ( ) from addendum
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Show component stack for invalid type warning during element creation
4 participants