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 router flow@0.53.x fixes #1214

Merged
merged 4 commits into from
Sep 8, 2017

Conversation

ajhyndman
Copy link
Contributor

These are a couple minor bugfixes to the react-router_v4 and react-router-dom_v4 typedefs.

Currently the props shape for the HOC withRouter constructs its internal prop shape with type intersection of inexact object. This works, but only if the inner component's props signature is an inexact object.

We can improve on this, by using an exact object type for ContextRouter and using type spread notation.

See also:

facebook/flow#2626

Our type signature should not assume that additional keys may be present in this object.
@lydell
Copy link

lydell commented Sep 6, 2017

Thanks! Manually editing my installed react-router-dom_v4.x.x.js with the patch from this PR fixed this problem for me:

Error: frontend/js/main.js:25
 25:           <RootRouter />
               ^^^^^^^^^^^^^^ React element `RootRouter`
 74: class RootRouter extends React.Component<Props, State> {
                                              ^^^^^ property `history`. Property not found in. See: frontend/js/RootRouter.js:74
 25:           <RootRouter />
               ^^^^^^^^^^^^^^ props of React element `RootRouter`

Error: frontend/js/main.js:25
 25:           <RootRouter />
               ^^^^^^^^^^^^^^ React element `RootRouter`
 74: class RootRouter extends React.Component<Props, State> {
                                              ^^^^^ property `location`. Property not found in. See: frontend/js/RootRouter.js:74
 25:           <RootRouter />
               ^^^^^^^^^^^^^^ props of React element `RootRouter`

Error: frontend/js/main.js:25
 25:           <RootRouter />
               ^^^^^^^^^^^^^^ React element `RootRouter`
 74: class RootRouter extends React.Component<Props, State> {
                                              ^^^^^ property `match`. Property not found in. See: frontend/js/RootRouter.js:74
 25:           <RootRouter />
               ^^^^^^^^^^^^^^ props of React element `RootRouter`

intersection of inexact object types behaves much like spreading, but when both objects are exact, the intersection type becomes an empty set.

See: facebook/flow#2626
@ajhyndman ajhyndman force-pushed the react-router-flow@0.53.x-fixes branch from b898a03 to a807d13 Compare September 6, 2017 12:12
@ajhyndman ajhyndman force-pushed the react-router-flow@0.53.x-fixes branch from a807d13 to 18e9122 Compare September 6, 2017 12:28
@marudor marudor merged commit 061be1f into flow-typed:master Sep 8, 2017
@johnryan
Copy link

@ajhyndman I've started seeing errors with code like this (note this is just an example otherwise it would be a pretty useless app):

class App extends Component<{}> {
  render() {
    return (
      <div>
        <Route path="/">
          <div />
        </Route>
      </div>
    );
  }
}
18:           <div />
               ^^^^^^^ exact type: object type. This type is incompatible with
133:     children?: React$ComponentType<ContextRouter>,
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ union: type application of polymorphic type: type `React$StatelessFunctionalComponent` | class type: type application of polymorphic type: class type: React$Component. See lib: flow-typed/npm/react-router-dom_v4.x.x.js:133
  Member 1:
  140:   | React$StatelessFunctionalComponent<Props>
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type application of polymorphic type: type `React$StatelessFunctionalComponent`. See lib:

It seems like this might be related to this PR...am I wrong? If so happy to open a separate issue

@johnryan
Copy link

nevermind my mistake after all, this fix just exposed me misusing the API..thanks!

@chrisdrackett
Copy link
Contributor

@ajhyndman I'm a bit confused, what needs to be done in a component to avoid issues on withRouter?

@ajhyndman
Copy link
Contributor Author

@chrisdrackett My intent with this PR was just to improve the typedefs so they work with a wider range of use cases without trouble. This PR did end up getting merged. Are you running into some specific issue still?

P.S. I like the avatar choice.

@chrisdrackett
Copy link
Contributor

chrisdrackett commented Oct 4, 2017

@aaronjensen I'm just having trouble getting withRouter to work with flow. When I do something like

export default withRouter(MyComponent)

flow complains:

class type: RouterLink. This type is incompatible with the expected param type of union: type application of type 'ClassComponent' | type application of type 'FunctionComponent'.

@ajhyndman
Copy link
Contributor Author

@chrisdrackett It sounds like flow doesn't think MyComponent is a valid component? It's hard to say much more without context.

@zachwolf
Copy link

I'm having an issue with these changes while using two HOCs. I've made a reduced example of the behavior.

I'm trying to use Material UI with Flow and react-router-dom. The component's export looks something like:

export default withStyles(stylesheet)(withRouter(Button))

and then Flow says:

$ flow
Launching Flow server for /Users/flow-react-router-dom-test
Spawned flow server (pid=79843)
Logs will go to /private/tmp/flow/XXXXXXXflow-react-router-dom-test.log
Library type error:
flow-typed/npm/react-router-dom_v4.x.x.js:144
144:     Component: React$ComponentType<{| ...ContextRouter, ...P |}>
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. This type is incompatible with
 14: class Button extends React.Component<Props> {
                                          ^^^^^ object type. See: src/Button.js:14
  Property `classes` is incompatible:
     19:           <Button content="click me" />
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ undefined. This type is incompatible with. See: src/App.js:19
                    v
      9:   classes: {
     10:     [string]: string,
     11:   },
           ^ object type. See: src/Button.js:9

Library type error:
flow-typed/npm/react-router-dom_v4.x.x.js:144
144:     Component: React$ComponentType<{| ...ContextRouter, ...P |}>
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. This type is incompatible with
 14: class Button extends React.Component<Props> {
                                          ^^^^^ object type. See: src/Button.js:14
  Property `content` is incompatible:
     19:           <Button content="click me" />
                                   ^^^^^^^^^^ undefined. This type is incompatible with. See: src/App.js:19
      8:   content: string,
                    ^^^^^^ string. See: src/Button.js:8

Error: src/App.js:19
 19:           <Button content="click me" />
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ React element `Button`
 19:           <Button content="click me" />
               ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ props of React element `Button`. Inexact type is incompatible with exact type
144:     Component: React$ComponentType<{| ...ContextRouter, ...P |}>
                                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ object type. See lib: flow-typed/npm/react-router-dom_v4.x.x.js:144

If I change the declaration to look like:

  declare export function withRouter<P>(
-    Component: React$ComponentType<{| ...ContextRouter, ...P |}>
+    Component: React$ComponentType<{ ...ContextRouter, ...P }>
  ): React$ComponentType<P>;

then it passes, but I'm not sure this is the right solution.

Any ideas what's going wrong here?

@ajhyndman
Copy link
Contributor Author

@zachwolf What do your flow types for withStyles look like? It could be some kind of conflicting interaction between the definitions.

@zachwolf
Copy link

@ajhyndman thanks for the response.

I think... it'd be:

const withStyles = (
  stylesOrCreator: Object,
  options?: Options = {},
): HigherOrderComponent<RequiredProps, InjectedProps> => (Component: any): any => {

Taken from mui

@ajhyndman
Copy link
Contributor Author

@zachwolf Aha! It looks like, internally, they are depending on another project for the HigherOrderComponent type:

https://github.com/digiaonline/react-flow-types/blob/develop/index.js.flow#L21-L24

And in this definition, they are still using intersection types, which won't handle exact objects, for the reasons I tried to outline above. I think you might be better served filing an issue with react-flow-types.

This issue looks related:

digiaonline/react-flow-types#9

@zachwolf
Copy link

@ajhyndman awesome response, thank you! I've opened an issue.

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

Successfully merging this pull request may close these issues.

None yet

6 participants