Flow type declarations test #878

wokalski · 2016-05-18T16:24:32Z

I'm using Flow with Immutable but I found numerous false positives when I was working with it.
By false positives I mean cases where types were ignored. Most of the time it resulted in types dropping their generic definition (from T<K,V> -> T).

This is WIP because:

tests for Iterable and Seq are not implemented yet
merge on maps ignores types when given a ESIterable. It only infers types from plain objects.
after initialization without type (for instance by calling Map()) and setting a value on empty object it doesn't correctly infer types

var map: Map<number, string> = Map().set(1, '')

This might be a flaw of Flow so feel free to point it here.

In f31e5c2 I copied the definition of $Iterable to immutable-js. I did it for two reasons:

It didn't work
$Iterable is private

Tests for Iterable and Seq are not implemented yet. I had some problems implementing tests for Iterable. Ideally I would like to cast some concrete type to it, but I'm not sure how to do it.

Is the type of Map<K,V> Iterable<K,V,void,void,void>?

Next steps:

If tests receive a 👍🏻 I'm going to implement tests for other types.
I believe I found a bug in Flow. Namely argument of union type is not correctly resolved. I am not sure how to phrase it in order to file a bug in flow. An instance of this problem can be seen here. ESIterable is casted to { [key: string]: T }. How do I name this, casting?

wokalski · 2016-05-18T18:43:46Z

pinging @marudor as a major contributor to this

marudor · 2016-05-20T19:09:19Z

type-definitions/immutable.js.flow

@@ -464,7 +485,7 @@ declare class List<T> extends IndexedCollection<T> {
 }

 declare class Map<K,V> extends KeyedCollection<K,V> {
-  static <K, V>(): Map<K, V>;
+  static <K,V>(_: void): Map<K,V>;


Whats the purpose of this?

Type inference didn't work on objects without it. It looked like flow somehow ignores the object type, falling back to the constructor without named parameters. Since parameters in both constructors are optional, I thought that simply removing the empty constructor would solve the problem but it didn't. I added this one, and honestly I copied it from here. It's hard for me to give further insight since I'm new to both js and flow.

I encourage to run the test without the patches first (to check out at d7ee48c) and then check out subsequent commits.

marudor · 2016-05-20T19:13:22Z

Added some comments - otherwise looks nice.

wokalski · 2016-05-20T22:44:23Z

Thanks for the review. I would like to note that the test for Map.merge doesn't pass because types are ignored. I would love to get some help fixing this.

wokalski · 2016-06-07T13:17:54Z

ping @marudor

wokalski · 2016-10-20T08:30:49Z

Closing. Work on this will be continued here

Also added flowconfig file to allow running the tests.

Fixes many errors in flow tests.

wokalski · 2016-12-05T13:00:42Z

I'm on a pretty epic bug. I wanted to fix imports according to this comment. The problem is that when I wrap everything in declare module everything is exported correctly, except for List. The funny thing is that if I rename List to Lista or really anything, it works.

lacker · 2016-12-05T23:54:04Z

.travis.yml


 env:
  global:
    - CXX=g++-4.9
    - secure: "VDsxy30sE9ivdqoXkaKXo0czbS4brNpwKEIblu7f1gVLx7OD9pjTc78cdwrVbZDBYroSiYVYuUrLDjpVjH88lL/LxRrru3V0CIlAqqa+ssXcqycCaT/6ds+ZymuTTGRh+Mf12pIKO+yc8jTov2M7AzPJdpS+ORP5dImYyE3ex9s="

+script: ./test_type_defs.sh


I'm probably missing something about Travis here, but what is this replacing? Like what was the test script entry point before script was set here? Just want to make sure we aren't omitting something now.

It's the first time I use node on travis and I felt that something was wrong, too. Thankfully @leebyron pointed out that npm install and then npm test is automatically ran by travis. There's no need to specify a script.

lacker · 2016-12-05T23:54:52Z

type-definitions/tests/.flowconfig

+.*/node_modules/.*
+
+[options]
+suppress_comment=^\\( \\|\n\\)*\\$ExpectError\\(([^)]*)\\)?$


What is this line doing? Is there a way to comment the .flowconfig to explain this?

leebyron

Perhaps update package.json to:

scripts: {
  "test": "everything-that-was-there && npm run type-check",
  "type-check": "flow check"
}

leebyron · 2016-12-05T23:53:53Z

.travis.yml


 env:
  global:
    - CXX=g++-4.9
    - secure: "VDsxy30sE9ivdqoXkaKXo0czbS4brNpwKEIblu7f1gVLx7OD9pjTc78cdwrVbZDBYroSiYVYuUrLDjpVjH88lL/LxRrru3V0CIlAqqa+ssXcqycCaT/6ds+ZymuTTGRh+Mf12pIKO+yc8jTov2M7AzPJdpS+ORP5dImYyE3ex9s="

+script: ./test_type_defs.sh


this would over-write the existing default script npm test - can we instead add the flow cli run to package.json scripts?

leebyron · 2016-12-05T23:54:18Z

test_type_defs.sh

+
+npm install
+cd type-definitions/tests
+../../node_modules/flow-bin/cli.js


also - when called from the context of npm scripts, node modules are added to PATH, so you could just write flow there.

TIL - thanks

lacker · 2016-12-05T23:57:12Z

type-definitions/tests/immutable-flow.js

+ * @flow
+ */
+
+// Some tests look like they are repeated but it's not actually the case; It is very likely to have a false positivie because Flow might not complain about an instance of (what it thinks is) T to be assigned to T<K, V>


"positive"

could you break this line up to keep to 80 char max or so

lacker · 2016-12-05T23:59:28Z

type-definitions/immutable.js.flow

+  declare function Repeat<T>(value: T, times?: number): IndexedSeq<T>;
+
+  //TODO: Once flow can extend normal Objects we can change this back to actually reflect Record behavior.
+  // For now fallback to any to not break existing Code


Code -> code

yea i know nitpicky

lacker · 2016-12-06T00:03:39Z

I just wanted to say that overall having flow type tests in this repo will make it a lot better, because it'll be a lot easier for us to accept a lot of the type-bug-fixes that have been submitted, so you are a hero for putting this PR together @wokalski

wokalski · 2016-12-06T09:17:58Z

thanks for the review - I broke CI and didn't know about it. The little things you mentioned should be fixed now.

lacker · 2016-12-06T18:09:32Z

Does $ExpectError actually fail if there is no error there or does it just allow an error on the subsequent line?

wokalski · 2016-12-06T18:20:03Z

Good point! It does fail.

lacker · 2016-12-06T18:30:30Z

YOLO

The flow type definition tests added in #878 relied on declaring a module, which required .flowconfig to explicitly include the flow definition files, and made exports unclear. This reverts some of the structural changes to the immutable.js.flow from that PR and simplifies the .flowconfig test file to ensure tests are against the *local* copy of immutable. This uncovered a few issues with tests including a mistaken sense that types were exportes as classes and that Set intersections were causing issues. Fixes #961

ghost added the CLA Signed label May 18, 2016

marudor reviewed May 20, 2016
View reviewed changes

ghost added the CLA Signed label Jul 12, 2016

wokalski closed this Oct 20, 2016

wokalski mentioned this pull request Dec 2, 2016

[Flow]: Add JS objects to Map.mergeIn & Map.mergeDeepIn #987

Merged

wokalski added 3 commits December 4, 2016 20:40

Added Flow declaration tests for List Map Stack and Set

ebbf17c

Also added flowconfig file to allow running the tests.

Replaced use of Flows private $Iterable with own Iterable definition

78811b4

Fixes many errors in flow tests.

Fixed Flow Map initializer declaration to work correctly with objects

61b02e8

wokalski reopened this Dec 4, 2016

wokalski force-pushed the feature/flow-declarations-test branch from ea50739 to d2fa889 Compare December 4, 2016 20:14

wokalski added 2 commits December 5, 2016 08:45

Makes travis run flow def tests

d290b72

Fixes flow definitions and tests

57b875c

wokalski force-pushed the feature/flow-declarations-test branch 2 times, most recently from 28f7f80 to b3ee572 Compare December 5, 2016 08:35

[travis] sets node version to 6

a2ea1c6

wokalski force-pushed the feature/flow-declarations-test branch 2 times, most recently from 3327572 to a2ea1c6 Compare December 5, 2016 08:43

Fixed flow declaration export to support default and named exports

373b781

wokalski changed the title ~~WIP: Flow type declarations test~~ Flow type declarations test Dec 5, 2016

Makes flow type tests ignore immutable npm module

17535a3

lacker self-assigned this Dec 5, 2016

lacker reviewed Dec 5, 2016

View reviewed changes

leebyron reviewed Dec 5, 2016

View reviewed changes

lacker reviewed Dec 5, 2016

View reviewed changes

lacker added the flow label Dec 6, 2016

wokalski added 2 commits December 6, 2016 06:54

Moves flow type testing logic to npm script

8892e14

Improves comments in flow tests and definitions

70def2d

lacker merged commit 0155f1c into immutable-js:master Dec 6, 2016

wokalski deleted the feature/flow-declarations-test branch December 6, 2016 18:32

wokalski mentioned this pull request Dec 6, 2016

[WIP] Definitions for immutable flow-typed/flow-typed#401

Closed

leebyron mentioned this pull request Mar 1, 2017

Test local flow type definitions #1079

Merged

BurntSushi mentioned this pull request Jul 24, 2017

add flow type definitions for immutable-js flow-typed/flow-typed#1068

Open

JoelMarcey unassigned lacker Feb 14, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flow type declarations test #878

Flow type declarations test #878

wokalski commented May 18, 2016 •

edited

wokalski commented May 18, 2016

marudor May 20, 2016

wokalski May 20, 2016

marudor commented May 20, 2016

wokalski commented May 20, 2016 •

edited

wokalski commented Jun 7, 2016

wokalski commented Oct 20, 2016

wokalski commented Dec 5, 2016 •

edited

lacker Dec 5, 2016

wokalski Dec 6, 2016

lacker Dec 5, 2016

leebyron left a comment •

edited

leebyron Dec 5, 2016

leebyron Dec 5, 2016

wokalski Dec 6, 2016

lacker Dec 5, 2016

lacker Dec 5, 2016

lacker commented Dec 6, 2016

wokalski commented Dec 6, 2016

lacker commented Dec 6, 2016

wokalski commented Dec 6, 2016

lacker commented Dec 6, 2016

Flow type declarations test #878

Flow type declarations test #878

Conversation

wokalski commented May 18, 2016 • edited

wokalski commented May 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marudor commented May 20, 2016

wokalski commented May 20, 2016 • edited

wokalski commented Jun 7, 2016

wokalski commented Oct 20, 2016

wokalski commented Dec 5, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leebyron left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lacker commented Dec 6, 2016

wokalski commented Dec 6, 2016

lacker commented Dec 6, 2016

wokalski commented Dec 6, 2016

lacker commented Dec 6, 2016

wokalski commented May 18, 2016 •

edited

wokalski commented May 20, 2016 •

edited

wokalski commented Dec 5, 2016 •

edited

leebyron left a comment •

edited