Fix flowtypes for OrderedSet, OrderedMap, toOrderedSet, and toOrderedMap #1027

howardjing · 2016-12-20T23:47:13Z

Unfortunately extending Set and Map caused issues with OrderedSet
and OrderedMap flow annotations. For example,

const orderedSet: OrderedSet<number> = OrderedSet.of(1,2,3);

was not valid, because of was incorrectly assumed to return a
Set<number> rather than an OrderedSet<number>.

The fix was to copy and paste the Set and Map flow annotations,
adjusting for OrderedSet and OrderedMap as appropriate.

See #1015 for more details.

Add negative flow test cases for OrderedSet and OrderedMap

One thing that came up as a result of this is that it seems
like the current flow definitions are laxer than typescript
definitions. For example, typescript's intersect looks like

intersect(...iterables: Array<T>[]): Set<T>

whereas flow's intersect looks like

intersect<U>(...iterables: ESIterable<U>[]): Set<T&U>

Also removed unnecessary OrderedSet overrides such as

delete
remove
clear

since we were already extending Set.

Also removed unused variables in immutable-flow tests.

Unfortunately extending Set and Map caused issues with OrderedSet and OrderedMap flow annotations. For example, ``` const orderedSet: OrderedSet<number> = OrderedSet.of(1,2,3); ``` was not valid, because `of` was incorrectly assumed to return a `Set<number>` rather than an `OrderedSet<number>`. The fix was to copy and paste the Set and Map flow annotations, adjusting for OrderedSet and OrderedMap as appropriate. See immutable-js#1015 for more details.

howardjing · 2016-12-20T23:51:04Z

If there's a way to fix this without copy pasting flow annotations for Map / Set, I'm happy to update this PR as necessary. I made this PR because with immutable 3.8.1 I couldn't use list.toOrderedSet with flow.

wokalski · 2016-12-21T13:35:08Z

type-definitions/immutable.js.flow


-  // OrderedMaps have nothing that Maps do not have. We do not need to override constructor & other statics
-  declare class OrderedMap<K,V> extends Map<K,V> {
+  declare class OrderedMap<K,V> extends KeyedCollection<K,V> {


Why did you decide to extend KeyedCollection? OrderedMap extends Map.

Ah, didn't realize that. I extendedKeyedCollection because Immutable.Map.isMap is a function whereas Immutable.OrderedMap.isMap doesn't seem to be a function:

I'll make the change for this and OrderedSet.

Turns out I'm getting an issue overriding update by extending Map.

I'll leave it as extending KeyedCollection for the purposes of this PR. Having OrderedSet extend Set worked without issues (I didn't realize I was already doing that), and I'll update the PR to remove extraneous OrderedSet definitions.

I think it's because of this in the update definition. Does it happen if you replace this on line 574 with OrderedMap<K,V> and with `Map<K,V> on line 580?

Unfortunately the updater function is still problematic. I can work around it by changing the update signature from

update<K_,V_>(updater: (value: Map<K,V>) => Map<K_,V_>): Map<K_,V_>;

to

update<K_,V_>(updater: (value: any) => any): Map<K_,V_>;

but I'm not sure how else to fix it. The error being generated is as follows:

inconsistent use of library definitions 571: update<K_,V_>(updater: (value: OrderedMap<K,V>) => OrderedMap<K_,V_>): OrderedMap<K_,V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ intersection. This type is incompatible with 500: update<K_,V_>(updater: (value: Map<K,V>) => Map<K_,V_>): Map<K_,V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type Member 1: 571: update<K_,V_>(updater: (value: OrderedMap<K,V>) => OrderedMap<K_,V_>): OrderedMap<K_,V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type Error: inconsistent use of library definitions 500: update<K_,V_>(updater: (value: Map<K,V>) => Map<K_,V_>): Map<K_,V_>; ^^^^^^^^^^ Map. This type is incompatible with 571: update<K_,V_>(updater: (value: OrderedMap<K,V>) => OrderedMap<K_,V_>): OrderedMap<K_,V_>; ^^^^^^^^^^^^^^^^^ OrderedMap Member 2: 572: update<V_>(key: K, updater: (value: V) => V_): OrderedMap<K,V|V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type Error: inconsistent use of library definitions 500: update<K_,V_>(updater: (value: Map<K,V>) => Map<K_,V_>): Map<K_,V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ undefined (too few arguments, expected default/rest parameters). This type is incompatible with 572: update<V_>(key: K, updater: (value: V) => V_): OrderedMap<K,V|V_>; ^^^^^^^^^^^^^^^^ function type Member 3: 573: update<V_>(key: K, notSetValue: V_, updater: (value: V) => V_): OrderedMap<K,V|V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ polymorphic type: function type Error: inconsistent use of library definitions 500: update<K_,V_>(updater: (value: Map<K,V>) => Map<K_,V_>): Map<K_,V_>; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ undefined (too few arguments, expected default/rest parameters). This type is incompatible with 573: update<V_>(key: K, notSetValue: V_, updater: (value: V) => V_): OrderedMap<K,V|V_>; ^^^^^^^^^^^^^^^^ function type

Extending KeyedCollection is better IMO.

wokalski · 2016-12-21T13:36:54Z

type-definitions/tests/immutable-flow.js

+orderedStringToNumber = OrderedMap({'a': 1}).mapKeys(x => x)
+orderedStringToNumber = OrderedMap({'a': 1}).flatten()
+orderedStringToNumber = OrderedMap({'a': 1}).flatten(1)
+orderedStringToNumber = OrderedMap({'a': 1}).flatten(true)


Thanks for the tests! It would be great if there were tests for expected errors, too. You can do it with // $ExpectError. See here.

wokalski · 2016-12-21T13:38:17Z

Good job. I think it's good to merge when there are tests for false positives ($ExpectError)

howardjing · 2016-12-21T22:54:47Z

I'm currently working on false positives, will update this PR tonight or tomorrow. I've found a few edge cases that I'll add TODOs / FIXMEs with explanations.

One thing that came up as a result of this is that it seems like the current flow definitions are laxer than typescript definitions. For example, typescript's intersect looks like ``` intersect(...iterables: Array<T>[]): Set<T> ``` whereas flow's intersect looks like ``` intersect<U>(...iterables: ESIterable<U>[]): Set<T&U> ``` Also removed unnecessary OrderedSet overrides such as * delete * remove * clear since we were already extending Set. Also removed unused variables in immutable-flow tests.

howardjing · 2016-12-22T01:27:18Z

Updated PR with negative test cases, and removed some unnecessary variables / overrides.

howardjing · 2016-12-22T01:29:28Z

The flow definitions seems to be a little laxer than the typescript definitions, I'm not sure if this is by design, so I left it alone.

wokalski · 2016-12-22T21:04:28Z

Out of curiosity - what do you exactly mean by laxer?

They are verbose, it's true. I don't have any experience with TS so I cannot compare. immutable is a bit special case because of generics and inheritance hierarchy.

wokalski

Thanks for the tests. They are very detailed. I love that. I think that it would be great if the issues with flow definitions landed in a collective issue. Maybe some of them are more controversial so they probably deserve their own issue.

Asking @lacker for an opinion since I'm just helping here

wokalski · 2016-12-22T21:09:43Z

type-definitions/immutable.js.flow

    static isOrderedSet(maybeOrderedSet: any): bool;

    add<U>(value: U): OrderedSet<T|U>;
-    delete(value: T): this;


wokalski · 2016-12-22T21:11:41Z

type-definitions/tests/immutable-flow.js

 // Some tests look like they are repeated in order to avoid false positives.
 // Flow might not complain about an instance of (what it thinks is) T to be assigned to T<K, V>

-import Immutable, {


Why is it deleted?

Immutable wasn't actually being used, so I got rid of it.

It tested valid imports. I know it might be confusing though. I should've put a comment there.

Interesting -- can you elaborate on that? I'll switch back to

import Immutable, { // ... } from 'immutable';

but I'm not clear as to what importingImmutable is testing. Should I add back

import * as Immutable2 from 'immutable' const ImmutableList = Immutable.List const ImmutableMap = Immutable.Map const ImmutableStack = Immutable.Stack const ImmutableSet = Immutable.Set const ImmutableKeyedIterable = Immutable.KeyedIterable const ImmutableRange = Immutable.Range const ImmutableRepeat = Immutable.Repeat const ImmutableIndexedSeq = Immutable.IndexedSeq const Immutable2List = Immutable2.List const Immutable2Map = Immutable2.Map const Immutable2Stack = Immutable2.Stack const Immutable2Set = Immutable2.Set const Immutable2KeyedIterable = Immutable2.KeyedIterable const Immutable2Range = Immutable2.Range const Immutable2Repeat = Immutable2.Repeat const Immutable2IndexedSeq = Immutable2.IndexedSeq

as well?

I made it - I know it's confusing. It tests if (1) the default import (2) the import * as X from 'immutable' (3) individual imports are supported.

Cool, I'll add that back and include a comment as to what it's testing. Thanks for the clarification.

wokalski · 2016-12-22T21:12:36Z

type-definitions/tests/immutable-flow.js

 // $ExpectError
 stringToNumber = Map([['a', 'b']])
+// FIXME: this should trigger an error -- this is actually a Map<string, string>
+stringToNumber = Map(List.of(List(['a', 'b'])))


Great find! Would you mind adding an issue for it? FIXMEs get lost pretty easily.

Yes, I'll create an issue with a list of flowtype wonkiness I've found.

wokalski · 2016-12-22T21:13:53Z

type-definitions/tests/immutable-flow.js

+ *    ...iterables: Iterable<K, V>[]
+ * ```
+ */
 stringToNumber = Map({'a': 1}).mergeWith((previous, next, key) => 1, [1])


I don't understand why it should throw an error :(.

iterable should look like a map rather than an array -- the following error is thrown when I run the snippet in the javascript console:

Ah! Gotcha.

wokalski · 2016-12-22T21:15:11Z

type-definitions/tests/immutable-flow.js

+ * This seems inconsistent with the typescript signature of
+ *
+ * ```
+ * update(updater: (value: Map<K, V>) => Map<K, V>): Map<K, V>


Thanks, great find again. Would you mind adding an issue (again)?

wokalski · 2016-12-22T21:17:23Z

type-definitions/immutable.js.flow

+    ): OrderedMap<K|K_,V|V_>;
+
+    mergeWith<K_,W,X>(
+      merger: (previous: V, next: W, key: number) => X,


Shouldn't key be of type K here? And K_ in the output?

I think that makes sense to me -- this was copy pasted from 2363 Map so this issue exists there as well. The other thing to note is that iterables should probably be ESIterable<K_,W> rather than ESIterable<W>[].

howardjing · 2016-12-23T01:32:21Z

To clarify on

The flow definitions seem to be a little laxer than the typescript definitions

I meant that methods that make persistent changes to the data structure in flow are allowed to introduce new types, whereas they are not allowed to introduce new types in typescript. For example, this is the Set#add method in flow:

class Set<T> {
  add<U>(value: U): Set<T|U>;
}

and this is the Set#add method in typescript:

interface Set<T> {
  add(value: T): Set<T>;
}

So we can do the following in flow

Set([1]).add('a');

but not in typescript. This behavior is also allowed in javascript for what it's worth. This seems like an inconsistency that I thought I'd bring up, and probably belongs in a separate issue.

howardjing · 2016-12-23T02:34:27Z

I made a gigantic issue here: #1029, let me know if you want me to split it up into mini issues.

wokalski · 2016-12-23T05:22:59Z

Great job. The only remaining issue are the mergeWith methods. Do you want to fix them here or should I or you create an issue for it? Other than that - thanks for helping all of us with this contribution.

howardjing · 2016-12-23T20:10:38Z

The mergeWith methods are included in #1029, but I'm working on that now and will update this PR with those changes.

There were some issues with the definitions of Map and OrderedMap's flowtype signatures for mergeWith and mergeDeepWith. 1. the merger function's `key` was incorrectly specified as being of type number 2. ...iterables was incorrectly specified to be array like rather than object like

howardjing · 2016-12-23T21:14:08Z

Updated with commits to add back the Import tests, and fix mergeWith and mergeDeepWith type signatures for Maps and OrderedMaps. I'm currently working on fixing updateIn for Maps / OrderedMaps in a separate commit.

The `updater` argument was specified as `any` rather than ``` (value: any) => any ```

howardjing · 2016-12-23T21:28:32Z

Updated with fix to updateIn

wokalski · 2016-12-23T22:41:46Z

@lacker I think it's good to merge.
@howardjing great job. Thanks for being so precise. Sorry for the nitpicking!

rvikmanis and others added 2 commits December 20, 2016 10:36

Fix toOrderedMap, toOrderedSet Flow annotations

aedd434

facebook-github-bot added the CLA Signed label Dec 20, 2016

howardjing mentioned this pull request Dec 20, 2016

Fix toOrderedMap, toOrderedSet Flow annotations #850

Closed

howardjing changed the title ~~Fix flowtypes~~ Fix flowtypes for OrderedSet, OrderedMap, toOrderedSet, and toOrderedMap Dec 20, 2016

howardjing mentioned this pull request Dec 21, 2016

Updated flow types #881

Closed

wokalski reviewed Dec 21, 2016

View reviewed changes

wokalski reviewed Dec 22, 2016

View reviewed changes

This was referenced Dec 23, 2016

Add "static" to flow type for OrderedSet<t> #996

Closed

Various flowtype issues with Map and OrderedMap #1029

Closed

howardjing added 2 commits December 23, 2016 15:33

Add back import tests

a9df9ba

Fix flowtype definitions for Map / OrderedMap's updateIn method

cfa0aac

The `updater` argument was specified as `any` rather than ``` (value: any) => any ```

lacker merged commit 29a5c55 into immutable-js:master Jan 9, 2017

Carlsson87 mentioned this pull request Jan 10, 2017

Build with the changes from pull request 1027 #1038

Merged

Kronuz mentioned this pull request Feb 15, 2017

Immutable update facebookarchive/draft-js#879

Closed

Uh oh!

Fix flowtypes for OrderedSet, OrderedMap, toOrderedSet, and toOrderedMap #1027

Fix flowtypes for OrderedSet, OrderedMap, toOrderedSet, and toOrderedMap #1027

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants