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

DefaultProps are not handled by higher-order components #4644

Closed
sphire opened this issue Aug 18, 2017 · 10 comments
Closed

DefaultProps are not handled by higher-order components #4644

sphire opened this issue Aug 18, 2017 · 10 comments
Labels

Comments

@sphire
Copy link

sphire commented Aug 18, 2017

If a component has some non-optional props that are covered by defaultProps and is then wrapped by a HOC, the defaultProps are not used correctly on the wrapped component. Flow will instead produce Property not found errors.

Version: 0.53.1

flow.org/try test case

@samwgoldman
Copy link
Member

This is an interesting example.

In your example, I can get the expected output by changing the return type of your component:

// Pseudo-HOC that just passes the component through
function pseudoHoc(Component: ComponentType<Props>): ComponentType<{a?:string}> {
  return (props) => <Component {...props} />;
}

Note that I changed the return type to ComponentType<a?:string>, effectively combining the props and default props, which I tend to call the "config" type, or the type of the object you need to pass into React.createClass.

However, we can't generalize this approach to an arbitrary component, because we need to know both the default props type and the props type to derive the config type.

cc @calebmer

@pigoz
Copy link

pigoz commented Aug 19, 2017

Is there any utility type we can use to automatically generate the "config" type without hardcoding it into the HOC definition?

Have you ever considered making Props for ES6 class components match the "config" type definition? (in the past it worked like that for stateless functional components by using default function parameters)

type Props = { a?: string } // valid as documentation for users of the X component
class X extends React.Component<Props> {
  static defaultProps = { a: true }; // inside the class this.props is refined to `{ a: string }`
}

@AriaMinaei
Copy link

AriaMinaei commented Aug 23, 2017

This is how I got it to work:

/* @flow */

import * as React from 'react';

// taken from https://github.com/digiaonline/react-flow-types

export type ComponentWithDefaultProps<DefaultProps: {}, Props: {}> =
  React.ComponentType<Props> & {defaultProps: DefaultProps};

/**
 * Here is a generic type for HOCs:
 *
 * It takes two type parameters: RequiredProps and ProvidedProps
 *
 * RequiredProps: The final wrapped component will need RequiredProps, in addition to its own props
 * ProvidedProps: The final wrapped component will not need ProvidedProps, because the HOC will provide them to the inner component
 */
export type HigherOrderComponent<RequiredProps: {}, ProvidedProps: {}> =
  & (<OriginalProps, DefaultProps>(component: ComponentWithDefaultProps<DefaultProps, OriginalProps>) =>
      React.ComponentType<RequiredProps & $Diff<$Diff<OriginalProps, ProvidedProps>, DefaultProps>>)
  & (<OriginalProps>(component: React.ComponentType<OriginalProps>) => React.ComponentType<RequiredProps & $Diff<OriginalProps, ProvidedProps>>);


type Props = { a: number, b: number };

class A extends React.Component<Props> {
  static defaultProps = { a: 1 };

  render() {
    return <div />
  }
}

// Pseudo-HOC that just passes the component through
const pseudoHoc: HigherOrderComponent<{}, {}> = (Component: any): any => {
  return (props) => <Component {...props} />;
}

<A b={10} />; // No errors, as expected
<A a={10} b={10} />; // No errors, as expected
<A a="some string" b={10} />; // Errors as expected

const B = pseudoHoc(A);
// correctly errors
<B b={10} />; // No errors, as expected
<B a={10} b={10} />; // No errors, as expected
<B a="some string" b={10} />; // Errors as expected

Try flow link

@rsolomon
Copy link

rsolomon commented Sep 1, 2017

Has anyone found a more concise solution? That's an enormous amount of code to write just to get basic, default behavior.

@AriaMinaei
Copy link

I think it would be a good idea if flow provided a generic HOC type for react, as long as it's not a hard-coded special type and that it's possible to make the same thing for other react-like libraries.

However, @rsolomo, the HigherOrderComponent type above is generic. You don't have to copy it for every component. And it's available through react-flow-types.

@rosskevin
Copy link

rosskevin commented Sep 27, 2017

This is something that breaks incremental adoption of flow.

I've used @AriaMinaei's HOC and it works fantastically for anything that is fully flow typed! Thanks @AriaMinaei - it exposed > 1700 errors on a codebase that was previously zero!

BUT

I now have a big problem, because it seems this HOC pattern assumes everything is flow typed. So at least half of those errors were lingering legacy prop-types in the material-ui lib.

e.g.

Error: src/GridList/GridListTileBar.spec.js:25
 25:       const wrapper = shallow(<GridListTileBar title={tileData.title} />);
                                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ React element `GridListTileBar`
                                 v
110: GridListTileBar.propTypes = {
111:   /**
112:    * An IconButton element to be used as secondary action target
...:
140: };
     ^ property `subtitle`. Property not found in. See: src/GridList/GridListTileBar.js:110
125:   propTypes?: $Subtype<{[_: $Keys<Props>]: any}>,
                                 ^^^^^^^^^^^^ props of React element `GridListTileBar`. See lib: /private/tmp/flow/flowlib_19598b07/react.js:125

The point it breaks in react.js:

122 declare type React$StatelessFunctionalComponent<Props> = {
123   (props: Props, context: any): React$Node,
124   displayName?: ?string,
125   propTypes?: $Subtype<{[_: $Keys<Props>]: any}>,
126   contextTypes?: any
127 };

This is a case where the a flow typed HOC wraps an old props-type component: withStyles(styles, { name: 'MuiGridListTileBar' })(GridListTileBar).

This breaks incremental adoption of flow. So while I continue to use this in the library and I'll simply convert the rest of our legacy files, I worry a bit about our flow shadow files breaking users of the library when they have not fully adopted flow.

@etrepum
Copy link

etrepum commented Oct 15, 2017

The issue we've run into with workarounds like what @AriaMinaei proposed above is that there doesn't appear to be a way to do the right thing when props is an exact object type. Either you use $Exact<…> in a few places and force all props types to be treated as exact (which complicates optional default props considerably; I haven't found a generic way to map {| a: T |} to {| a?: T |}) or you leave it as-is and lose support for exact props type checking altogether.

@tjmehta
Copy link
Contributor

tjmehta commented Nov 17, 2017

Had to tweak the above example for flow v0.59

https://flow.org/try/#0PQKgBAAgZgNg9gdzCYAoVBLAtgBzgJwBdkwBDAZzACUBTUgY2KnzizAHJ87H2BudYMDCFSAaxoA7MM1ZgAFoUI5yALkEBzDITkBXAEYA6eq2AATDJtJwJMDBJrAuDQgFpYiF4QCeOGuXQ0AB54RMI+NGAAwqx49hKEAOpacgAiNFCkOjCEAAosygA8aRlZufmqYADeAL4ANGB5cMoqVdUAfGAAvKhg1NyEBtG41pKEACrhBY3KHQBkVabpmdnTFcXLZU3k1fyooCA94AASNFxgGJSkYOqSpxj0Yb7SBGBHAPKRqoeHYACSxCJxJRCAg4I8IjhSPhSFgaIRThVaABHHQYLimVZkCSmBosABuGEWGPK3xIyNR6NWLTGcgiUDspBgYAQ0JwvhxxmGcWICAwMCZ9hoOPJaKFq3qdjIpnMhAw1mEYK0lEQUhwJJIjQJRKpYBpdIZTJZpDZQrAnNio2ZfIFcGIgpxmsJYvK9T0NHomXIEW0EXekSt-LAargWu9tLYhDBPvOEns+DNMRG8UOaCCIQB4VeFlp+De+EW+CGFviBRFlPKLRq9Ud2orrQ63V68wAFAU8xYDeKwOtSqs2s3zUnCC0i0OktoeytykUlr2XWB25oJIy+wBKLptHq9be0ZyDRPcia+Us0FGi4lbMDzAAkKQwUCgBVv98fi8785rzq2bXqk82MyvMBrwAZTkY0aDbfAO2XGA+zaVctxbSDoJXcp+0HbkWl3Rh9y5UYjwgt8YLXDcwC3bc+j3UdD0mMsv2UQDnwfZCl1QrZq3xJ0LxmeDdlQbwnkxToqjIFoJB0LA3XwV0xIkqSwB2dB6BgChKAAQTAIJ4WxShsIGajRimNCqi3cgRFlB5FhKKdL2EypRLAABGBTdl6LhsVOZt10qciwC4QgdHwKQCnMPEwGATdemqVBoooijfPi5TVKiTTAm00xdP6XDi0IIzvxMtzJALLyCu3fzAuC0LwsihSYoEIQci9HRTDgFw-WEMDiAAKx0Myg1UvwOoiDDLW0FgdHUORUGMCQ+uUGhmrgI44HoFojmzU48wLAySyresujAZsduHLEvFXFpSAkLxSJ8wqAqCw7g2UddOg6ApjqqAwvqe7Zqv4aLUAKDS9E6SpHIABmqP7wqEAA5MFThYfByHqChUt8RghUBjTSFBiGoZBsHIehwQwHhzT8CRlGyEoNN3XhUw9iEIGyE6AAichWAiMyoIkdQ2bAQn8ZJoQAFFKYIS5aeCemsem6w+oAIQO+bFuW+hmzU1d+FJ4xKfpmBrsRyXAeVoXiYi3gYbJhGJeR1HpYxhnTdZomCbxi22it0nyeN+2afR2XGdJgpldxjmubAHm7H5wWPahy3rfFqmA7pzHGYS7dAf9XG3bjvPE5922qYdwP06ZsB3vz4XC7Fu3qbRtOGbirOQ5z9nOdhKPCF52PzYTr2k-r1OZfTrP5dm4gUhVpqWvVo7tcz3pAen3Phf7kWbYpkuR6drGilZtmoDgOABY32uwGTyXS6boUgA

@TLadd
Copy link

TLadd commented Apr 12, 2018

The docs cover how to solve this issue now using React.ElementConfig: https://flow.org/en/docs/react/hoc/#toc-supporting-defaultprops-with-react-elementconfig

@jamesisaac
Copy link
Contributor

I believe this is solved from Flow v0.89 with the new React.AbstractComponent type: https://medium.com/flow-type/supporting-react-forwardref-and-beyond-f8dd88f35544

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests