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

Maybe union type ordering error #5320

Closed
dylanapplegate opened this issue Nov 11, 2017 · 6 comments
Closed

Maybe union type ordering error #5320

dylanapplegate opened this issue Nov 11, 2017 · 6 comments

Comments

@dylanapplegate
Copy link

Expected Result
Maybe union types can be specified in any order:
?(Array<T> | T)
?(T | Array<T>)

Actual Result
The ordering does matter. Specifying the array first works correctly. While including the generic first causes an error.

I first noticed this error in React using flow version 0.53.1, but the examples provided above are for v0.59.0

@bradennapier
Copy link

Hey @deanbrophy this is because T could be an array so T in this case matches everything while Array<T> only matches arrays of T.

Therefore you always want to order your types from most specific to least so it doesn't match an unwanted type.

@dylanapplegate
Copy link
Author

@bradennapier Excellent! That makes sense. I'm wondering if the error message could be more specific, though, or is this something that's explicit in the documentation?

@bradennapier
Copy link

bradennapier commented Nov 11, 2017

Actually upon reading this error it is actually different from what I assumed:

forEachAccumulated(myList, doThis);
                           ^ function. This type is incompatible with the expected param type of
                           cb: (elem: T) => void,
                                 ^ function type
                                 This parameter is incompatible:
forEachAccumulated(myList, doThis);
                    ^ array type. This type is incompatible with
                      function doThis(something: Whatever){
                                      ^ object type

Basically it is saying that it is epxecting the Whatever object but instead it is receiving ?(Whatever | Array<Whatever>)

Indicating that we will be passing a non-array type in the annotation seems to fix this.

Try Flow

/* @flow */

type $NonArrayType<A> = $Call<(<T>(Array<T> | T) => T), A>  
    
function forEachAccumulated<T>(
  arr: ?(T | Array<T>),
  cb: (elem: $NonArrayType<T>) => void,
  scope: ?any,
) {
  if (Array.isArray(arr)) {
    arr.forEach(cb, scope);
  } else if (arr) {
    cb.call(scope, arr);
  }
}

type Whatever = {
  Stuff: number
}

var myList: ?(Whatever | Array<Whatever>) = null;

function doStuff(){
 	forEachAccumulated(myList, doThis);
}

// Without $NonArrayType<T> this function is expecting 
// { Stuff: number } | Array<{ Stuff: number }> 
// -- but you had specifically annotated it to be 
//    { Stuff: number } - so it did not type properly.
function doThis(something: Whatever) {
   // $Works
  (something: { Stuff: number }); 
  // $ExpectError 
  //(something: [{ Stuff: 1 }, { Stuff: 2 }]);
}

This should provide the perfect illustration of why your example provided an error:

Try Flow

// this is what you were giving the function but had it annotated as `Whatever`
function doThis(something: Whatever | Array<Whatever>){}

Yes, there is no question that the errors that Flow provides are pretty hard to decipher. In many cases they show up in the wrong places and are masked by other errors or problems. I would imagine that is a very difficult thing to handle properly -- here's to hoping they continually refine such things over time ;-).

@dylanapplegate
Copy link
Author

Right on! I think it's safe to close out this issue then.

Man, I need to get caught up on utility types, but they're not very well documented. I had no idea $Call existed.

@bradennapier
Copy link

@deanbrophy you can take a look at our pubchan package. It has some useful utility types for composing your types: https://github.com/Dash-OS/pubchan/blob/master/src/pubchan/types/utils.js

@dylanapplegate
Copy link
Author

Right on. I wish the built-in Flow utility types were better documented. It looks like things like $Call have no documentation whatsoever.

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

No branches or pull requests

2 participants