Composing multiple HOCs that return ComponentTypes using ElementConfig breaks prop types

[oliver-stripe]

To reproduce: https://flow.org/try/#0JYWwDg9gTgLgBAKjgQwM5wEoFNkGN4BmUEIcA5FDvmQNwBQduEAdqvE+BKlgFxwAkAYRKRucALxwAFMwCuAG3l9kzAJ4BKeoxZs4wZgCss+AGIQIEuAB4ACsTCo+AbwC+AGjgBpPtjwwAdMKczFjMMAAqqmBYtvaoAHzxUnRwcEGQIWF8nm506j5UAeksoRFRMfwAIsAEBFa++P4AovJYIKXCzATAAOZWnvEeTgTmfGxQ+j1wAD5wAG4QwAAmLokS8XBOKXCUMLJQzNJyispqmnQuWkys8PpG+ABCyFCWsRAOzu5eBX6BIiVhSLRN4ORLJVLFTIwbK5fKYQp-YKlIEVaq1eoIlptDosbp9AZDABGzzGMAmzCmswWy1WG3EGy2qV2+0OMgUShQZ3olwYuHkaHQTQAHshwK1IaU4FghTBQkt0A0iv8oVZGXARhBSeSerlUsSoFrJrq4MSAF58OQgQlYKDG7rmk3mVoqXKrTbbNjIGDAXBwJZYAjIBQwOzvdCSNV65AOgAMxp5LgYAHok2kABbIfRYJZpZXI8pJrHtMKdPEmyjIADW6DAcW0NzgAElDMZZUsJWFLBxRFhwXoW6ZzMa7q2nra8lJhaKwOK82FzqlF0vl3QUwJhdF8E0oMQXoTZPBmBAAv5-HBUGmIAoc93+foUISr4RgKa4CBgKhUEaTcgc2az7WYYoJQ55YG0kzyKocAAAYqKo0FwEeADudBWM29xth28BmuIABEACMABMADMuFwEm8RaGuDwHnABiyLoJRwEh0DVqe-j1ro6GttmWFmBYkgjoOECTiKYpYFh5xoQOmFzjA-E-lAeFEaR6ovuRlHJqmzZsDgOYQAQOxYHsByTLmSKAuUHg1MxWDqtAuB2YI-KfhivxYVY7GJNZ8AsVA1acbcMn8YRryhh8mxfN48LuXJKIggkSTbFhMJ5HwzkClY2yKoiGSlFYVQ1HUOVFjiXS9P0gybBqhoUjM8yLCsVVwYMdB0gy2zMgcRzsqcGjcvQy5DcNgX9hhY6hZICWfB40U5Vh8XhYlfYpV4sLpS5qBZak81yQVaLFZirTFjApYVQSmz6rVlINTSzVqK17Xukyxksj1Jycv1FyDcNv1DaN3H4LxcmTXA3ZcL22xCfJ5iEcOMkTbConTrOFkwOcq6pvwG6ttuu6oYDslo6FOEESRZH2hpVGpgA6qxqAAIQEzJwPE2pr4UTQQA

I've attempted to comment the above. Some more detail:

• injectFoo and injectBar are HOCs that return React.ComponentType calculated using React.ElementConfig. Alone, they work as expected. However, when composed together, the resulting component loses all prop types.
• injectFoo2 and injectBar2 are alternate typed versions of the same HOC. Instead of returning React.ComponentType they return just Class<React.Component<...>>. Composing these together works as desired.

[vetvicka]

Simple example of this issue. I'm using the myHOC component from the documentation (https://flow.org/en/docs/react/hoc/#toc-supporting-defaultprops-with-react-elementconfig).

https://flow.org/try/#0PTAEAEDMBsHsHcBQBLAtgB1gJwC6gFSgCGAzqAEoCmRAxnpFrKqAORbV0uKIig2zpklACagGTUMNg0SiSAFcAdnWSxFoVAE8AEgHkAwgB4ACo3QkANKH1NMiyopwAuChxwA6GxjUOcAFU10ShMzEgA+MIAKRFBQAHUsInQg4S87Xxc0n0dEAEoXKloPLPtHAKDDQrp3AFFoSlRfG0VIZABzQxLfCNAAbxjQdhx5LHV0UNAAXjDQQwSklK7HPvdV8YESAF9QYDCAbkRN7hxAylBTDam+gfX0AEEXRXlUACNKLAsB+GQcAAsAEUokCI8mgzlAJBwWGQijanyO-EUkNAAGU0Oh6ks8JNQJFbiQXBdzLkpjNDMJkAA3MK9fHuW53Ta00Lub5-QHA0E4TaGYAU6mINEYTG2bIeYRAkFgolkHH9WJsgGSrkuABE2ko0Dgqvh3ERyJqil+RGUIixVy0en0kSFGMoWNyB31eENxtNqVFpRwACYLToDJFLQHbSLvF7co69WpkThKMicZFyVSwgNDCH7Z7fKAGZNegBGba7VOuk00M2Z5a7PrAQg2LDsOjQTSgP6MeBkGr17AEYBHWKGEvurG+nYzXo11G-WCg0SthCgTuMLA9o68-lhR1AA

[cesardeazevedo]

I've be able to work around this by wrapping the ElementConfig in a $Exact, but i am still unsure about any side effect of it

try it

[cesardeazevedo]

Here's a better approach with $Supertype instead of $Exact. try it

[Blasz]

@cesardeazevedo Unfortunately, those two workarounds don't work when the HOC has another argument:

$Exact

$Supertype

Nevermind, they work fine with multiple args, they were failing because I was hoisting the return prop type into a generic for readability (#6587):

$Exact

$Supertype

[jamesisaac]

It looks like, with the release of Flow 0.89.0, and its new AbstractComponent type (Docs, Medium), which ComponentType now aliases to, the examples in the first 2 posts now work correctly, hooray!

However, strangely, they can't be rewritten in the new way actually suggested in the docs, or it breaks composition again.

Example created by rewriting injectBar from example in first post to way it's shown in docs. Error on line 35 no longer gets picked up.

cc @jbrown215

[jbrown215]

Out of the office today, so I won’t be able to look at this in more detail until Monday. If you rewrite without using $Compose does it work? I expect there may be a bug with $Compose.

Glad to see this solved other issues here, though!

[jamesisaac]

The issue still remains when rewriting from $Compose to injectFoo(injectBar(ExampleComponent));, unfortunately. (Same with these issues in previous Flow versions, testing without $Compose never seemed to make any difference).

Thanks a lot for taking a look at this. Not being able to reliably compose HOCs has been the only major pain point I've experienced with Flow when trying to scale it out to large React projects.

[jbrown215]

Ah, in that case I think I know what the issue here is. If I’m correct, the issue is with $Diff not receiving the bounds necessary from the function call.

If you give explicit type arguments to the HOCs does it work as expected?

[jamesisaac]

Aha, yes, it does indeed work if written as:

injectFoo(injectBar<React.Config<Props, DefaultProps>>(ExampleComponent))

(where only injectBar has been rewritten to the new style).

The failure was silent though, no "missing annotation" error as mentioned in the docs.

Is needing the explicit type arguments by design, or a bug? If intentional, maybe it would be better to continue defining HOCs in the old style, if it means less typing work for the user when they actually come to use them? It also seems that explicit type arguments wouldn't work with $Compose. Or is there some other disadvantage to the older style, apart from it being more verbose?

[jbrown215]

The problem here is actually the same as the root cause of this issue— when type destructors don’t receive the necessary bounds they have strange behavior. ElementConfig could also have been used with explicit type arguments before 0.89.

I have some work in the pipeline with @samwgoldman to ask for annotations in these cases. This work is also explicitly on our roadmap for the next half.

For now, I recommend sticking with the new style and adding type arguments when you can.

[jbrown215]

I should also mention that if you export the component it will ask for the annotations already. In my experience, most of the time these components that get wrapped in multiple HOCs are exported. This shouldn’t be too silent most of the time :)

But we definitely will make sure it’s always flagged, even when it isn’t exported.

[jamesisaac]

Trying out everything in the new style, it seems that the examples are back to correctly picking up errors again, even with no explicit types. I think my issue was using a mixture of old style and new style -- sorry, should have thought to try all HOCs as new style.

However, testing it out with exported components, it seems that even then the Missing type annotation error is absent (basically having a $Diff in the HOC return type stops the error appearing, so only trivialHOC that adds no props triggers that error). So I'm not sure if my usage will start triggering an error in future, because I have no explicit types.

My concern with needing explicit types for every HOC, is that it will add a lot of complexity for the types when writing otherwise simple components, especially for the average developer who only has limited Flow knowledge. Every level of HOC would need a $Diff, where you know exactly which props were removed. Something like:

export default injectFoo<$Diff<React.Config<Props, DefaultProps>, { bar: string | void }>>(
  injectBar<React.Config<Props, DefaultProps>>(MyComponent)
);

... just imagine with 3 levels of HOC, would get unwieldy extremely quickly. Support for implicit/inferred types, at least for the outer HOCs, would make this a lot more manageable imo.

[jbrown215]

My concern with needing explicit types for every HOC, is that it will add a lot of complexity for the types when writing otherwise simple components, especially for the average developer who only has limited Flow knowledge

Couldn't agree more. The most important annotation is the outermost, and you don't have to give annotations as type arguments. For example:

const export: React.AbstractComponent<SomeConfig> = wrap1(wrap2(wrap3(...(wrapN(Component))...);

Even so, there is still room for improvement here. I don't expect flow to get any less strict with annotation requirements in the future, so there is definitely meaningful work to be done in helping reduce the burden these annotations cause.