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

Write about Flow not accepting more strict Arrays #9

Open
niieani opened this issue Feb 2, 2017 · 12 comments
Open

Write about Flow not accepting more strict Arrays #9

niieani opened this issue Feb 2, 2017 · 12 comments

Comments

@niieani
Copy link
Owner

niieani commented Feb 2, 2017

function test(param : Array<number | string>) {
  return param
}

const compatibleVar : Array<number> = [1, 2, 3]

test(compatibleVar)

This works properly in TypeScript, but fails in Flow with the error:

3: function test(param : Array<number | string>) {
                                        ^ string. This type is incompatible with
7: const compatibleVar : Array<number> = [1, 2, 3]
                                ^ number
@niieani niieani changed the title Write about lower quality inference in Flow Write about buggy inference in Flow Feb 2, 2017
@niieani niieani changed the title Write about buggy inference in Flow Write about Flow not accepting more strict Arrays Feb 8, 2017
@vkurchatkin
Copy link

Just to clarify: Flow is being more strict here, not TS

@niieani
Copy link
Owner Author

niieani commented Feb 10, 2017

@vkurchatkin I know it's being more strict, I just can't think of a reason why would you want this behavior. Can you give a reason as to why this is better?

@vkurchatkin
Copy link

This is better because it's safe. Typescript's behavior is unsafe:

function test(param: Array<number | string>) {
  param.push('foo')
}

const compatibleVar : Array<number> = []

compatibleVar[0].toFixed(); // Runtime error

@niieani
Copy link
Owner Author

niieani commented Feb 10, 2017

@vkurchatkin ok, this makes sense (I'll add it to the README), but only if you're mutating data. It would be nice to be able to opt-in or opt-out of this, since with modern programming / immutability / functional dogma one really shouldn't be doing what test in the above example does (i.e. mutating the parameters).

I also think what you gave as an example is an edge case, so personally, I believe TypeScript's trade-off for a bit of unsoundness goes a long way in making it easier to develop in general.

(btw. you missed one line: test(compatibleVar), but I inferred what you were trying to say :))

@vkurchatkin
Copy link

Well, in Flow you can just do:

function test(param: $ReadOnlyArray<number | string>) {
}

And it's going to work

@niieani
Copy link
Owner Author

niieani commented Feb 10, 2017

That's great to know, however this is an undocumented feature. :(

@leoasis
Copy link

leoasis commented Feb 11, 2017

Maybe this blog post can be helpful regarding this particular Flow behavior? https://medium.com/@forbeslindesay/covariance-and-contravariance-c3b43d805611#.6oemuj9gn

Unless you're already familiar with it, in any case maybe it helps for posterity.

@niieani
Copy link
Owner Author

niieani commented Feb 11, 2017

@leoasis I was aware of variance and how to use it (it's documented), it's only the $ReadOnlyArray that I didn't know about :) But thanks, the article is pretty useful!

@RyanCavanaugh
Copy link

Probably good to talk about Flow's treatment of mutated arrays in the same section:

const arr = ['1', 0];
arr.sort();
// Error in TS, not in Flow, throws at runtime
arr[0].substr(0); 

@vkurchatkin
Copy link

Interesting example, I've already filed this issue here: facebook/flow#3735

The thing is, Typescript's behaviour seems to be not that different. In your example TS only catches the error because it doesn't try to infer precise type of arr. But if you add an annotation it's going to allow the same behaviour:

const arr: [string, number] = ['1', 0];
arr.sort();
arr[0].substr(0)

Flow, on the other hand, will show an error in this case.

@RyanCavanaugh
Copy link

Flow, on the other hand, will show an error in this case.

Only because tuples are read-only. The equivalent TS code would be:

const arr: ReadonlyArray<number | string> = ['1', 0];
arr.sort();

or

const arr: { 0: string, 1: number } = ['1', 0];
arr.sort();

which both show the same error.

Probably TS tuples should inherit from some other Array type that allows mutation but doesn't allow reordering operations. Allowing sort / splice / etc on [number, string] is certainly wrong.

@vkurchatkin
Copy link

Only because tuples are read-only. The equivalent TS code would be:

No, they are not really read-only, they just have methods of $ReadOnlyArray.

Probably TS tuples should inherit from some other Array type that allows mutation but doesn't allow reordering operations. Allowing sort / splice / etc on [number, string] is certainly wrong.

That's basically what Flow does

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

4 participants