Flow: Intersection with AbstractComponent leads to misleading errors

[omninonsense]

Flow version: 0.104

Expected behavior

Don't error when the intersection can be rendered as a React component.

Alternatively, if this is another strange design decision regarding React.AbstractComponent, can we at least error early (as soon as we try to intersect with AbstractComponent) instead of in a random place in the code.

Actual behavior

Flow complains that React.Component is not a React component when it appears in an intersection with AbstractComponent.

This is a kind of dumbed down example: https://flow.org/try/#0PTAEAEDMBsHsHcBQiCWBbADrATgF1AFSgCGAzqAEoCmxAxvpNrGqAOTY32vIigAqACxTlhoAK6kqAE1BVok+AKodQKAHahcSzQE8M6gOalEtaGXIBJNbmVYzuYgCNoVAMLMsaqtYA8ABQA+WQAPGzUpcmo6XAASd0xYL19A0ABvRFBQUlwdFyl4z29cCykALizcbEMAbkQAX2QcjCpQAGUcvILEov8mDFIAGlArbOI1WioggF5KTliAQUds7GiupNxe2H6hkYdxydAAMmHrW1h7Jxc1nrGdAOQpKlNiFUgxcdwURNA0YgBrKibbYnUb7AIACgAlOV2rlpNdkn1BiC9hMArUTIlsqACqAZr8AT5UnUhqwpCgAG6sCGQ5C0LH4RzEfIwjrwjzdXzE0mkDBjal40A+XHAAJAA

However, a real-world example of this can be found in flow-typed's typedefs for styled-components: https://github.com/flow-typed/flow-typed/pull/3517/files#diff-2b77bbf95ac6caa4414db8ad19a62162 (search for should render as the correct element for the whole test that reproduces this, and for noExpectError for the immediate line below it that triggers the error).

More information about it can be found in https://github.com/flow-typed/flow-typed/pull/3517#issuecomment-523005033 (and some subsequent comments).

/cc @goodmind

[goodmind]

/cc @jbrown215

[jbrown215]

Alternatively, if this is another strange design decision regarding React.AbstractComponent

If it's by design, I stopped caring, because the Flow devs are stubborn af.

If you're going to be snarky and rude, at least be correct.

The way you're referring to InterpolatableComponent in StyledComponent is referring to an instance of the class. An instance of InterpolatableComponent is not a React component-- the class is. I agree that the error message leaves much to be desired.

Here, I fixed it for you.

Alternatively, if this is another strange design decision regarding React.AbstractComponent

If you're referring to the fact that we don't carry defaultProps around, you should consider more kinds of React components. AbstractComponent is meant to model _all_ of them, and not all of them have defaultProps. Moreover, defaultProps are being deprecated on function components. Maybe what you interpret as a "strange design decision" is actually a decision made that considered more information than you did.

[goodmind]

I still can't get this to work

[goodmind]

This probably isn't related tho

[goodmind]

It works only if I specify generic explicitly, but no inference

[omninonsense]

Alternatively, if this is another strange design decision regarding React.AbstractComponent

If you're referring to the fact that we don't carry defaultProps around, you should consider more kinds of React components. AbstractComponent is meant to model _all_ of them, and not all of them have defaultProps. Moreover, defaultProps are being deprecated on function components. Maybe what you interpret as a "strange design decision" is actually a decision made that considered more information than you did.

As far as I can tell, the feature is not deprecated _yet_, it is _going to be_ deprecated. As it currently stands, Flow _removed_ a feature at the type level that still exists, and is used in a lot of existing libraries, application code and tools.

The fact we're using _a tweet_ as a reference point feels ridiculous. The only other official source I can find is a MR that adds a disabled warning, this being mention in a few issues. I—for the life of me—can not find this properly documented _anywhere_. Documentation in Flow for this present-incompatible, futuristic behaviour should be the bare minimum.

Anyway, I don't want to go off-topic more than we already did. If you think it's a good idea, I can open a PR (or an issue) about documenting this.

If you're going to be snarky and rude, at least be correct.

I am slightly frustrated with the way Flow team generally communicates (and the documentation in certain places). The frustration was slightly elevated when I wrote that comment. None of this is really a justification for my behaviour; I realise the rudeness doesn't help, I regret being rude. I apologise to you and everyone else on the Flow team.

Rudeness is uncalled for regardless of whether I was right or wrong technically.

---

The way you're referring to InterpolatableComponent in StyledComponent is referring to an instance of the class. An instance of InterpolatableComponent is not a React component-- the class is. I agree that the error message leaves much to be desired.

Here, I fixed it for you.

Thank you! 😄

---

Once again, I really am genuinely sorry for being rude, and upsetting/offending you, or anyone else. Thank you for all the work you're doing! ❤️

[jbrown215]

Thanks, I appreciate your apology. My reaction could have also been more tame, but people in open source can often be rude and every now and then the humanity in me shines through in terrible ways :P.

I can agree that it's weird to use a tweet as a reference. But even _without_ the deprecation on defaultProps, AbstractComponent still should not model defaultProps. Do Fragment, ConcurrentMode, Suspense, the wrappers returned from lazy, memo, and all the other React wrappers have default props? If so, then a reasonable case can be made to support them. If not, AbstractComponent is more "abstract" than what you would want to model when you're using it.

But still, think about what code would look like if you had that third type argument:

React.AbstractComponent<Props, DefaultProps, Instance>. What would the type of React.forwardRef even look like? You can't specify defaultProps when you create a forwardRef component, so this wouldn't be sound:

  declare export function forwardRef<Props, DefaultProps, Instance>(
    render: (
      props: Config,
      ref: { current: null | Instance, ... } | ((null | Instance) => mixed),
    ) => React$Node,
  ): React$AbstractComponent<Props, DefaultProps, Instance>;

This signature would only be sound assuming you always remembered to actually set the default props afterwards. If you're still doubtful about this, I recommend writing code that assumes AbstractComponent tracked defaultProps separately. I think you'll find that it gets cumbersome, and very hard to make sound libdefs.

I am slightly frustrated with the way Flow team generally communicates

That's fair and understandable. We don't do it nearly as frequently as the community deserves.

[omninonsense]

Thanks, I appreciate your apology. My reaction could have also been more tame, but people in open source can often be rude and every now and then the humanity in me shines through in terrible ways :P.

I can agree that it's weird to use a tweet as a reference. But even _without_ the deprecation on defaultProps, AbstractComponent still should not model defaultProps. Do Fragment, ConcurrentMode, Suspense, the wrappers returned from lazy, memo, and all the other React wrappers have default props? If so, then a reasonable case can be made to support them. If not, AbstractComponent is more "abstract" than what you would want to model when you're using it.

But still, think about what code would look like if you had that third type argument:

React.AbstractComponent<Props, DefaultProps, Instance>. What would the type of React.forwardRef even look like? You can't specify defaultProps when you create a forwardRef component, so this wouldn't be sound:

  declare export function forwardRef<Props, DefaultProps, Instance>(
    render: (
      props: Config,
      ref: { current: null | Instance, ... } | ((null | Instance) => mixed),
    ) => React$Node,
  ): React$AbstractComponent<Props, DefaultProps, Instance>;

This signature would only be sound assuming you always remembered to actually set the default props afterwards. If you're still doubtful about this, I recommend writing code that assumes AbstractComponent tracked defaultProps separately. I think you'll find that it gets cumbersome, and very hard to make sound libdefs.

Thanks for going out of your way to explain this; the whole decision makes a lot more sense to me now!

[jbrown215]

No problem, sorry it's taken me so long to write this all up. Also for completeness: https://github.com/facebook/react/pull/16210