Skip to content

Commit

Permalink
Fixed useCallback definition (and tests)
Browse files Browse the repository at this point in the history
  • Loading branch information
Brian Vaughn committed Nov 28, 2018
1 parent dabf49e commit e33b4b5
Show file tree
Hide file tree
Showing 10 changed files with 661 additions and 1,143 deletions.
6 changes: 3 additions & 3 deletions lib/react.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -323,10 +323,10 @@ declare module react {
inputs: ?$ReadOnlyArray<mixed>,
): void;

declare export function useCallback<T>(
callback: () => T | void,
declare export function useCallback<T, U>(
callback: (...args: T) => U,

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Nov 28, 2018

Contributor

I'm not sure what you mean. Wrong link?

Replacing the args type with $ReadOnlyArray seems to break typing for the function params.

This comment has been minimized.

Sign in to view

Copy link
@jbrown215

jbrown215 Nov 28, 2018

Contributor

https://flow.org/try/#0ATAmFMGMBsEMCdzHADwA4Ht4BdgDMBXAO0mwEsMjgCBncAYVmmgCNZIBrAHgBUAaYAFUAfAAoAUCBCQmrdhwBcwUQDo1CAOY0lAEgBK4WKADyRaAE8AgvHixzvYQEpgAXmFC+kqWSJoC2bWAAfn1DEzMrGzsuAFsyFHBQYU8QRyVVdXgtJR5nNyEAbnFxWgZZNk4uIgIYlnB4AWra+rFRFCUmuobgcw6arrz3FEcCoA Here's the version using $ReadOnlyArray.

You want T to be the type of each element in the varargs, not the type of the varargs itself right?

Which part broke with function params?

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Nov 28, 2018

Contributor

Thank goodness for the Flow REPL 😅

This comment has been minimized.

Sign in to view

Copy link
@jbrown215

jbrown215 Nov 28, 2018

Contributor

The second type argument is for the return type. If you want to take string or number as arguments to the function you can set the first type argument to number | string. Unfortunately, we can't really do more granular types for each argument, since flow doesn't support variadic generics.

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Nov 28, 2018 •
edited
Loading

Contributor

Oh I see.

In that case, why is ReadOnlyArray an improvement over my original type suggestion? It handles the more granular check.

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Nov 28, 2018 •
edited
Loading

Contributor

ReadOnlyArray feels more technically correct but it doesn't feel like the correct practical choice (then again, I'm new here).

Edit why do I keep typing "ReactOnlyArray" heh

This comment has been minimized.

Sign in to view

Copy link
@jbrown215

jbrown215 Nov 28, 2018 •
edited
Loading

Contributor

Great question. That definition digs at a hole in flow. Specifically, if not given concrete type arguments then the type of callback is determined by its just. Put your cursor on callback on line 6. Notice the type of callback on the bottom left of the screen. Then delete the last line and check the type again-- you'll noticed it's changed!

This is a property we don't like in flow. In the future, we will ask for annotations on functions that have this kind of behavior. I think I did figure out how to do a little better and also get the inference algorithm happy:
https://flow.org/try/#0CYUwxgNghgTiAEIAeAHA9jALvAZgVwDsxMBLNA+PAZxAGEoIIAjKMAawB4AVALngAoAdMNgBzKnwAkAJRBRgAeQIQAngEEYMKCo4gAtikwqAfAEp4AXmPw9JJCGDH+AKHjwwDZqzZ8uAGld4EgIUPEwJeAB+GTlFZXVNbQ5be0cA018AbmdnMHIqbA9GFnZLShp6Yu9+fgI8PT46vSYQGD94Apg+TuDRcyt4AG9nAF9TbKKvdn4ARgAmAGZ2gHIoJjBl8fgAem34BTZczxK2fmX5heWVtY2t3fgAUU0MbswYXqCqIKI0AyhSJgQBAAdxImAAFvAmi0YIIgA

Here, the type of T is a function. ...empty => mixed is the most general type of function, so it will ensure that the type parameter is a function. If you put your cursor back on line 6, you'll notice it correctly infers the type!

This comment has been minimized.

Sign in to view

Copy link
@jbrown215

jbrown215 Nov 28, 2018

Contributor

I don't think we need two type arguments

If you were to explicitly annotate useCallback you would need to type out the full type of the function either way.

That's also the main downside to this strategy. However, as of today, the only time you would need to annotate the function is if it's exported. I can't imagine that someone would want to export the result of useCallback, so I don't think we need to worry about that too much. Also, it wouldn't be flagged as something that leaks tvars in the future because it receives adequate bounds from the arguments

This comment has been minimized.

Sign in to view

Copy link
@bvaughn

bvaughn Nov 28, 2018

Contributor

Huh. I'm sorry! I guess I messed up my initial test because I thought your previous one didn't work for that. 😄 Thanks!
inputs: ?$ReadOnlyArray<mixed>,
): T;
): (...args: T) => U;

declare export function useMemo<T>(
create: () => T,
Expand Down
18 changes: 4 additions & 14 deletions tests/getters_and_setters/getters_and_setters.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -465,13 +465,8 @@ References:
react.js:17:13
17| (<Example a="bad" />); // error: number ~> string
^^^^^ [1]
<<<<<<< HEAD
<BUILTINS>/react.js:404:36
404| number: React$PropType$Primitive<number>;
=======
<BUILTINS>/react.js:403:36
403| number: React$PropType$Primitive<number>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [2]


Expand All @@ -487,13 +482,8 @@ References:
react.js:18:20
18| (<Example a={0} c={0} />); // error: number ~> string
^ [1]
<<<<<<< HEAD
<BUILTINS>/react.js:406:36
406| string: React$PropType$Primitive<string>;
=======
<BUILTINS>/react.js:405:36
405| string: React$PropType$Primitive<string>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]


Expand Down
100 changes: 23 additions & 77 deletions tests/new_react/new_react.exp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -189,13 +189,8 @@ Cannot assign `this.props.x` to `_` because number [1] is incompatible with stri
^^^^^^^^^^^^

References:
<<<<<<< HEAD
<BUILTINS>/react.js:404:36
404| number: React$PropType$Primitive<number>;
=======
<BUILTINS>/react.js:403:36
403| number: React$PropType$Primitive<number>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [1]
classes.js:57:12
57| var _: string = this.props.x;
Expand Down Expand Up @@ -393,13 +388,8 @@ Cannot assign `this.props.z` to `qux` because:
^^^^^^^^^^^^

References:
<<<<<<< HEAD
<BUILTINS>/react.js:404:36
404| number: React$PropType$Primitive<number>;
=======
<BUILTINS>/react.js:403:36
403| number: React$PropType$Primitive<number>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [1]
new_react.js:19:18
19| var qux: string = this.props.z;
Expand All @@ -415,13 +405,8 @@ Cannot assign `this.props.x` to `w` because string [1] is incompatible with numb
^^^^^^^^^^^^

References:
<<<<<<< HEAD
<BUILTINS>/react.js:406:36
406| string: React$PropType$Primitive<string>;
=======
<BUILTINS>/react.js:405:36
405| string: React$PropType$Primitive<string>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [1]
new_react.js:20:15
20| var w:number = this.props.x;
Expand Down Expand Up @@ -454,13 +439,8 @@ References:
new_react.js:29:23
29| var element = <C x = {0}/>;
^ [1]
<<<<<<< HEAD
<BUILTINS>/react.js:406:36
406| string: React$PropType$Primitive<string>;
=======
<BUILTINS>/react.js:405:36
405| string: React$PropType$Primitive<string>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]


Expand Down Expand Up @@ -565,13 +545,8 @@ Cannot assign `this.props.x` to `a` because:
^^^^^^^^^^^^

References:
<<<<<<< HEAD
<BUILTINS>/react.js:406:36
406| string: React$PropType$Primitive<string>;
=======
<BUILTINS>/react.js:405:36
405| string: React$PropType$Primitive<string>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [1]
props.js:14:16
14| var a: number = this.props.x; // error
Expand Down Expand Up @@ -607,13 +582,8 @@ Cannot assign `this.props.z` to `c` because:
^^^^^^^^^^^^

References:
<<<<<<< HEAD
<BUILTINS>/react.js:404:36
404| number: React$PropType$Primitive<number>;
=======
<BUILTINS>/react.js:403:36
403| number: React$PropType$Primitive<number>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [1]
props.js:16:16
16| var c: string = this.props.z; // error
Expand All @@ -634,24 +604,14 @@ References:
props.js:20:29
20| var element = <TestProps x={false} y={false} z={false} />; // 3 errors
^^^^^ [1]
<<<<<<< HEAD
<BUILTINS>/react.js:406:36
406| string: React$PropType$Primitive<string>;
=======
<BUILTINS>/react.js:405:36
405| string: React$PropType$Primitive<string>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]
props.js:20:49
20| var element = <TestProps x={false} y={false} z={false} />; // 3 errors
^^^^^ [3]
<<<<<<< HEAD
<BUILTINS>/react.js:404:36
404| number: React$PropType$Primitive<number>;
=======
<BUILTINS>/react.js:403:36
403| number: React$PropType$Primitive<number>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:411:36
411| number: React$PropType$Primitive<number>;
^^^^^^ [4]


Expand Down Expand Up @@ -708,13 +668,8 @@ References:
props2.js:9:41
9| getInitialState: function(): { bar: number } {
^^^^^^ [1]
<<<<<<< HEAD
<BUILTINS>/react.js:406:36
406| string: React$PropType$Primitive<string>;
=======
<BUILTINS>/react.js:405:36
405| string: React$PropType$Primitive<string>;
>>>>>>> Add React.Suspense component and tests
<BUILTINS>/react.js:413:36
413| string: React$PropType$Primitive<string>;
^^^^^^ [2]
props2.js:15:42
15| return <C {...this.state} foo = {0} />;
Expand Down Expand Up @@ -745,21 +700,12 @@ Cannot get `React.PropTypes.string.inRequired` because property `inRequired` is
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

References:
<<<<<<< HEAD
<BUILTINS>/react.js:380:39
<BUILTINS>/react.js:387:39
v
380| type ReactPropsChainableTypeChecker = {
381| isRequired: ReactPropsCheckType;
382| (props: any, propName: string, componentName: string, href?: string): ?Error;
383| };
=======
<BUILTINS>/react.js:379:39
v
379| type ReactPropsChainableTypeChecker = {
380| isRequired: ReactPropsCheckType;
381| (props: any, propName: string, componentName: string, href?: string): ?Error;
382| };
>>>>>>> Add React.Suspense component and tests
387| type ReactPropsChainableTypeChecker = {
388| isRequired: ReactPropsCheckType;
389| (props: any, propName: string, componentName: string, href?: string): ?Error;
390| };
^ [1]


Expand Down
Loading

0 comments on commit e33b4b5

Please sign in to comment.