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

react/require-render-return doesn't account for switch statement #543

Closed
okize opened this issue Apr 12, 2016 · 5 comments
Closed

react/require-render-return doesn't account for switch statement #543

okize opened this issue Apr 12, 2016 · 5 comments
Labels

Comments

@okize
Copy link

okize commented Apr 12, 2016

This example throws error Your render method should have return statement even though an element is being returned.

class Header extends React.Component {
  render() {
    const { text, headerType } = this.props;

    switch (headerType) {
      case 'h1':
        return <h1 className="large-header">{text}</h1>;
      case 'h2':
        return <h2 className="med-header">{text}</h2>;
      default:
        return <h3 className="sm-header">{text}</h3>;
    }
  }
}
@ljharb ljharb added the bug label Apr 12, 2016
@jseminck
Copy link
Contributor

If the return is wrapped in an if statement then it also doesn't work:

render() {
        var test = true;

        if (test) {
            return <div></div>;
        }
        else {
            return <div></div>;
        }
    }

@jseminck
Copy link
Contributor

I tried to fix this in jseminck@2a51f23 and jseminck@0bf8e3f

It now works with switch and if statements. I also added tests for a nested if and switch statement.

Additionally added another commit that makes sure that every child of the if and switch statements has a return statement:

@yannickcr @ljharb would be good if any of you could take a look at the code as I'm not really all that familiar with ASTs. It was quite challenging (but definitely a fun exercise).

@ljharb
Copy link
Member

ljharb commented Apr 14, 2016

The tests look good (although the error messages could be more helpful/tailored to the error condition) - I'm not up to the task of reviewing the AST stuff.

@jseminck
Copy link
Contributor

I'll look at improving the error message(s) and documentation and then make a PR.

Also I ran into the same issue reported in #542 already, e.g. that render() methods in classes that do not extend React.Component also are linted. Perhaps I'll have a look at that too as it is also stopping us from being able to use this rule.

@yannickcr
Copy link
Member

Thanks for the details and the fix, but I rewrote the rule to use the component detection and fix #542 at the same time.

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

No branches or pull requests

4 participants