Fix issues with IntersectionTypeAnnotation for prop-types and no-unused-prop-types

[jseminck]

Closes #1413

3 cases needed to be covered:

• Mutliple intersections:

type PropsA = {}
type PropsB = {}
type PropsC = {}
type PropsD = PropsA & PropsB
type Props = PropsC & PropsD

• Importing types. This was already handled for other cases, but not for the intersection.
• type Props = PropsA & { foo: string }

[ljharb]

i can't decide if this style, or early-returning each place this var is assigned, is better

[jseminck]

Honestly I'm in general not really happy about the fact that a function with a name declarePropTypes*** returns a boolean at all. It's a bit confusing. That's why I liked this style a bit more, because it's very explicit to say what those methods return and what the boolean actually means.

Perhaps a try/catch may be even better?

[ljharb]

We could rename the function instead :-)

[ljharb]

since this is reducing to a boolean, could it use some or every, and that way it will bail early?

[ljharb]

instead of reusing types here, can we use multiple vars and use const?

[jseminck]

This is from the existing code so didn't touch it yet. But I think we can simply do:

const types = buildTypeAnnotationDeclarationTypes(value, key) || {};
declaredPropTypes.push({
  ...types,
  fullName: key,
  name: key,
  node: value
});

Edit: Ah! We don't support spread operator yet. 😞

[jseminck]

Meh, another problem is that buildTypeAnnotationDeclarationTypes returns either a boolean or an object which isn't great either. I try to refactor it to always return an object, but it needs changes in more places than just here.

But I do think it's a good refactor to do 😄

[jseminck]

Made some additional changes. Did a smoke test on the material-ui code which is currently breaking with rc0. I changed those functions to return early, but haven't changed the name as I couldn't come up with something.

@yannickcr please also have a look as this is required for 7.4.0.

[ljharb]

Thanks, this looks much better!

[ljharb]

What is the typeof guarding against here? This allows null, for example.

[jseminck]

null would be checked in the if statement before:

if (!propType) {
    // If it's a computed property, we can't make any further analysis, but is valid
    return key === '__COMPUTED_PROP__';
}

Otherwise, if I remove the typeof condition, one test starts failing. Turns out propType is a function() in one of the cases (not sure why) and it satifies the condition Object.keys(propType).length === 0. Perhaps should use https://www.npmjs.com/package/lodash.isplainobject ?

function Hello(props) {
  return <div>{props.name.constructor.firstname}</div>
}
Hello.propTypes = {
  name: PropTypes.shape({
    firstname: PropTypes.object
  })
};:

AssertionError: Should have 1 error but had 0: []

Basically we want to check if its {} instead of true, since those methods always return an object now instead of a boolean.

[ljharb]

Gotcha; makes sense.

[ljharb]

Can these two functions live somewhere they can be shared across the rules that currently duplicate them?

[jseminck]

Yes. I'm working on it 😄 The idea will be that the rule will be declared as:

create: propTypes.detect((context, components, utils) => {
...
})

And this will take care of ALL the shared code between prop-types and no-unused-prop-types. Perhaps also for the other rules.