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

Chore: Add tests for prop-types destructuring #2029

Merged
merged 1 commit into from Dec 27, 2018

Conversation

sstern6
Copy link
Contributor

@sstern6 sstern6 commented Oct 26, 2018

Fixes and verifies: #1422

@MatthewHerbst
Copy link
Contributor

MatthewHerbst commented Oct 26, 2018

@sstern6 Would it be possible to add a deep-destructuring example as well? For example:

class MyComponent extends React.PureComponent {
  propTypes = {
    foo: PropTypes.shape({
      // TODO: fill this out
    }).isRequired,
  }

  render() {
    const { foo: { bar } } = this.props;
    return <div>{bar}</div>;
  }
}

Expected error would be: \'bar\' is missing in props validation'

I ask for this because I've seen cases where sometimes destructuring works on the outer layer but not when you go deeper.

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 26, 2018

@MatthewHerbst just checked locally and looks like with the example:

class MyComponent extends React.Component {
  static displayName = 'MyComponent';
  static propTypes = {
    foo: PropTypes.shape({}),
  }

  render() {
    const { foo: { bar } } = this.props;
    return (
      <div>
        {bar}
      </div>
    );
  }
}

Does not throw any errors.

This PR was to test for spread destructruing and the errors associated with that. I can open a new ticket that will include the test you asked for and the fix, how does that sound?

How do we feel about this?

@MatthewHerbst
Copy link
Contributor

@sstern6 #1422 actually reports the problem noted above, which is why I was hoping to see a deep-destructuring test in the test suite that purportedly tested it heh. Seems #1422 still isn't fixed though. I think the tests you have above are good to add regardless, but I don't think they should be said to fully fix and/or verify #1422

@sstern6
Copy link
Contributor Author

sstern6 commented Oct 26, 2018

Youre right sorry about that, ill get on a fix for the deep destructuring, got caught up in the spread operator examples.

@MatthewHerbst
Copy link
Contributor

Amazing, thank you! If there's any way I can help, please let me know. I've taken a glance at it before and didn't really know where to start, but happy to look again.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

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

LGTM, but it seems like i should wait for a few additional tests to be added.

@ljharb ljharb merged commit a442067 into jsx-eslint:master Dec 27, 2018
This was referenced Dec 28, 2018
This was referenced Jan 4, 2019
@ghost ghost mentioned this pull request Jan 12, 2019
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants