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

React.Component defaultProps not validated by flow #3499

Closed
EvHaus opened this issue Mar 11, 2017 · 8 comments
Closed

React.Component defaultProps not validated by flow #3499

EvHaus opened this issue Mar 11, 2017 · 8 comments

Comments

@EvHaus
Copy link

EvHaus commented Mar 11, 2017

Using flow 0.41.0, with the following code:

import React, {Component} from 'react';

export default class MyComponent extends Component {
	props: {
		someValue: string
	};

	static defaultProps = {
		someValue: 123
	};

	render () { ... }
}

I expected flow to alert me that someValue is defined as a Number when it should be string. But it does not warn me at all.

@nmn nmn added the bug label Mar 11, 2017
@nmn
Copy link
Contributor

nmn commented Mar 11, 2017

$Diff should error if the second param is not strictly a sub-object of the first param.

@mwalkerwells
Copy link
Contributor

@nmn Which $Diff are you talking about? Looking at the library definition, props and defaultProps are not inter-dependent: https://github.com/facebook/flow/blob/master/lib/react.js

Also, you could make the case that a polymorphic class like this should infer that the type is { someValue: 123 | string }.

@nmn
Copy link
Contributor

nmn commented Mar 14, 2017

@mwalkerwells $Diff is used to create the prop requirements for a React Component. This way props that exist as defaultProps don't need to passed at the call-site.

Also, the types here are not inferred they are defined for props so inferring { someValue: 123 | string } makes no sense.


That said I'm investigating a possible fix that would validate that props and defaultProps are compatible.

- declare class React$Component<DefaultProps, Props, State> { ... }
+ declare class React$Component<DefaultProps: $Shape<Props>, Props, State> { ... }

This may or may not work.

@mwalkerwells
Copy link
Contributor

mwalkerwells commented Mar 14, 2017

$Diff is used to create the prop requirements for a React Component.

Can you link to this in the React.Component library definition?

@nmn
Copy link
Contributor

nmn commented Mar 14, 2017

@mwalkerwells It's kind of in the Flow internals. But this gets close: https://github.com/facebook/flow/blob/master/lib/react.js#L97

@asolove
Copy link
Contributor

asolove commented Aug 22, 2017

There were a number of issues with the relationship of props and default props that have been fixed in Flow v0.53.

I slightly updated the original code and it now type-checks fine in Flow v0.53.

As a result, I think this can now be closed. (/cc @calebmer)

@calebmer
Copy link
Contributor

One thing worth noting is that we only check defaultProps when you create an element from your component. This is ok:

import * as React from 'react';

class MyComponent extends React.Component<{prop: number}> {
  static defaultProps = {
    prop: 'foo',
  };
}

However, you get an error when you add <MyComponent />. This mirrors React’s runtime behavior 😊

https://flow.org/try/#0JYWwDg9gTgLgBAKjgQwM5wEoFNkGN4BmUEIcA5FDvmQNwBQduANmugLICeAwiZAHZY+8LAA8YggCbpseGADoe4CAKEAeAN5hiYAFxw+AVxAAjLFAC+APjjq6cOKhjIYwXHAlYCyA0xgAFbXQAXhs7ezgtCF1yAggIMgAaMPN6cwZVTkV+QXgAeksaIA

@fragosti
Copy link

@calebmer I'm confused about "mirrors React's runtime behavior" being a good thing. Seems like it should be possible to check this type of thing without instantiating a component? This come up a lot if you have React component packages.

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

No branches or pull requests

7 participants