Javascript: The "react/default-props-match-prop-types" setting forces looser typing

[callumlocke]

If you use the optional Flow Linter, you can get warnings for 'sketchy' checks, meaning when you do something like if (foo) or foo ? ... : ... when foo is not clearly typed. Example:

type Props = {
  isHappy?: boolean,
};

const Emoji = ({ isHappy }: Props) => 
  <span>{isHappy ? '😀' : '😭'}</span>;
//       ^^^^^^^  Sketchy null check on boolean which is potentially false. Perhaps you meant to check for null or undefined?

Emoji.defaultProps = {
  isHappy: false,
};

The linter thinks isHappy might be undefined, because we have annotated it as a maybe-boolean, but in fact it's guaranteed to be boolean.

So in fact the Props definition above is wrong; it should just beisHappy: boolean, because the props argument passed into the function is guaranteed always to have a boolean isHappy value when you render via JSX, thanks to the defaultProps. The linter is not at fault here, we're just confusing it by actively loosening the type for a prop that needn't be loosened.

You can fix the linter warning by changing the Props to have isHappy: boolean. (And you can still then render <Emoji /> without type errors, thanks to the defaultProps.) But doing this breaks compatibility with this eslint-config-airbnb rule:

'react/default-props-match-prop-types': ['error', { allowRequiredDefaults: false }],

This ESLint rule seems, at first glance, to encourage stricter code. But it actually forces you to loosen Flow's knowledge your props' types. This causes Flow Linter warnings and may lead to real refinement problems down the line.

So I propose changing it to ['error', { allowRequiredDefaults: true }] to allow stricter props typing. The meaning of defaultProps is well established, and it needn't be visually reinforced with corresponding maybe-types, especially if those maybe-types actually weaken Flow's understanding of what's going on.

[ljharb]

Airbnb doesn’t use flow, for one, so this might be a change you have to make in your own config.

Separately, changing the type here is incorrect - the ? means that the consumer of the component can choose to omit the value - it’s not about what value ends up inside the component.

In other words, if this is the change Flow is demanding here, then Flow has a bug - perhaps you want a separate Props type for the propTypes, and another one to cast the Props to for internal usage? I’m not really certain. Please file an issue there.

[callumlocke]

Separately, changing the type here is incorrect - the ? means that the _consumer_ of the component can choose to omit the value - it’s not about what value ends up inside the component.

That was my first instinct (and I did originally report this to Flow thinking it was a bug there) but I now think this is a misunderstanding.

Sure, if I call it as a plain function (Emoji({});), then it's going to be a Flow error, because I didn't supply a valid props object.

But that function is never consumed by calling it directly. It's consumed like this:

<Emoji />

// ...which compiles to:
React.createElement(Emoji, null);

The internals of React.createElement guarantee to call your function with a complete, merged props object. If you're consuming it via React.createElement (which is obviously the use case this ESLint rule is supposed to cover), then the Props type is entirely about what ends up inside the component, after merging.

[ljharb]

I don't agree; props match propTypes, and propTypes reflect the external API.

You're welcome to disable the rule if you don't like it - but it's not appropriate to have a defaultProp on a required prop - the whole point of adding a type, or propTypes, is to force the user to supply the required prop, or else they get a propType/flow error.

[callumlocke]

to _force_ the user to supply the required prop, or else they get a propType/flow error.

But it doesn't force the user to supply it, proof here. In that demo the prop is required, yet doing <Emoji /> causes no type error. This is correct behaviour, because by doing <Emoji /> I _am_ actually passing a prop (and Flow can figure this out), it's just not so obvious from the JSX syntax.

Sorry to press on this, I'm happy to override the rule myself, I just don't get what exactly you disagree on here... Do you get my point that the expression <Emoji /> actually compiles and unpacks to (roughly) Emoji({ ...Emoji.defaultProps, ...passedProps })?

[ljharb]

When it's a propType - imo the gold standard - it issues a warning in non-production, which any sensible test system will cause to fail tests. Obviously at runtime, you can do anything you want.

In your demo, that's a bug in Flow combined with React - it explicitly means that a required flow type combined with a defaultProp makes it implicitly optional, which is a flat out failure of the type system imo.

Default props are an implementation detail - propTypes/prop types are not. The former is not exposed to consumers, and should not be, and refactoring of them should always be safe - the latter is absolutely exposed to consumers, and dictates the API (the props) of the component.

To restate: I completely agree with your assessment of how things are working at runtime, and in your demo - but that doesn't alter the conceptual purpose of prop types, which are to determine what the consumer is allowed to provide.

[callumlocke]

I appreciate you taking the time to debate with me on this and I hope you're finding it as interesting as I am :) I want to try one last thing to convince you...

Try this in tryflow:

import React from 'react';
type Props = { isHappy: boolean };
const Emoji = ({ isHappy }: Props) => <span>{isHappy ? '😀' : '😭'}</span>;
Emoji.defaultProps = { isHappy: false };

Emoji({ ...Emoji.defaultProps, ...null }); // VALID (1)

React.createElement(Emoji, null); // VALID (2)

<Emoji />; // VALID (3)

Emoji({}); // TYPE ERROR!

We can both agree the last line is a legitimate type error, as the argument doesn't match Props.

The lines numbered 1-3 are all considered valid, and you stated this is a bug in Flow. I think it is correct in all cases. One by one:

1. This line is clearly valid, as you can statically prove the argument matches Props.
2. This line is effectively the same as [1], because if you follow the flow into React's internals (as Flow does) you can statically prove that that the Emoji function is eventually called with an argument that matches Props.
3. This is just alternative syntax for [2], so Flow should treat it identically to [2].

Do you disagree with any of the above statements? If so, which one(s)?

[ljharb]

Line 1 is clearly valid - you're passing all the required props, and all props passed have the proper type.

Line 3 is clearly invalid.

Line 2, however, is not the same as Line 1 or Line 3 - and prop types should be applied to the provided props - NOT to the "props the component receives", which because of defaultProps, is a different thing. It's not trying to prove that Emoji is eventually called with the right types, as would be true for a normal function - it's trying to prove that the user passes the right props to an Emoji element, which is different, and happens before defaults are applied.

In other words, type checks should be applied before defaults are applied (which is the case in React, at least when you omit the default).

[callumlocke]

OK. I'm going to close this, I still think there's an issue, but I think the issue is initially in the ESLint rule's design, so I might try to pursue it over there. Thanks for discussing this with me, I know you probably spend a lot of time fielding issues, and this eslint config is by far the best imo :)

I still believe all 3 lines are the same from Flow's perspective. I don't understand how you can say that line 2 is not the same as line 3. It is literally just different syntax for an identical expression. It would be a very surprising design decision if Flow treated them differently.

(btw I'm not clear on whether you think line 2 should be a type error?)

I think you are working under the assumption that using a type Props = {...} is supposed be just a direct static equivalent to React's .propTypes, but they're not. They overlap in purpose, and it would be unusual to use both together, but they do assert different things.

I believe the ESLint rule's allowRequiredDefaults option should allow a new option, 'auto', which should be the default, and it should effectively mean true for a Flow annotation and false for propTypes.

[callumlocke]

@ljharb I just found the Flow docs back me up:

React also supports default props on stateless functional components. Similarly to class components, default props for stateless functional components will work without any extra type annotations.

import * as React from 'react';

type Props = {
  foo: number, // foo is required.
};

function MyComponent(props: Props) {}

MyComponent.defaultProps = {
  foo: 42, // ...but we have a default prop for foo.
};

// So we don't need to include foo.
<MyComponent />;

It is 100% intended behaviour.

[ljharb]

That it’s intended doesn’t mean it’s not a design flaw - Flow is incorrectly interpreting how prop types should work in React.

[callumlocke]

OK, you were calling it a bug until now. You may see it as a design flaw. But the fact is it works that way, and it's intentional. Any tooling that claims to support Flow (like eslint-plugin-react) should support it based on how it actually works, or not at all.

FWIW, the React project seems to fully support Flow's design here, and they export utility types that embrace the distinction: React.ElementConfig works out a component's 'config' type (to match attributes in a JSX/React.createElement expression), while React.ElementProps gets you the type of the 'props' object passed into the function. These two types are treated and discussed as different things, in both the Flow docs and the React docs. It's how things are and it's not going to change. Personally I think it's a useful distinction to make.

[ljharb]

That's interesting. So it sounds like propTypes more closely match ElementConfig - where ElementProps are your internal props.

So, in that case, I'd say that the react plugin should be requiring ElementConfig, and not just "typed props" - and then all the same rules should apply. ElementProps is something that should be entirely optional.

[callumlocke]

Yeah that sounds like a good solution

[ljharb]

Woud you mind filing an issue about this on eslint-plugin-react, including flow background info, so we can shift to requiring ElementConfigs instead of ElementProps?