Error in the new version of flow 0.55

[szagi3891]

Library in version : immutable@4.0.0-rc.2

To get this error just run flow:

Error: node_modules/immutable/dist/immutable.js.flow:519
519:   static of<T>(...values: T[]): SetSeq<T>;
                                     ^^^^^^^^^ SetSeq. This type is incompatible with
383:   static of<T>(...values: T[]): IndexedSeq<T>;
                                     ^^^^^^^^^^^^^ IndexedSeq

[samwgoldman]

@leebyron Oops, sorry I didn't upstream a fix for this, but internally I resolved this in D5708450. On that diff, I wrote:

The Immutable API for sets has a fatal flaw, because Set is defined to be a
subtype of Collection<T,T>. This makes it impossible to define methods on the
base class which modify either K or V in a type-safe way.

We previously ran into this exact issue with Immutable.Iterable.map, which
takes an Iterable<K,V> and a function V => M and produces an
Iterable<K,M>. This creates a problem, because Immutable.Set<T> is declared
to be a subtype of Immutable.Iterable<T,T>. Here's an example of the problem:

function f(iter: Iterable<string, string>): Iterable<string, number> {
  // what does this return? if we pass a set, this function's type is wrong
  // if we pass any other iterable, this function's type is OK
  return iter.map(s => s.length);
}

We fixed this by removing the declaration of Iterable.map and instead
declaring the signatures on every subclass.

This issue follows the same pattern. If B is a subtype of A, then
Class<B> is a subtype of Class<A>.

function f<K,V>(S: Class<Seq<K,V>>, ...values: V[]): Seq.Indexed<V> {
  return S.of(...values);
}

// We think this returns Seq.Indexed<string> (Seq<number,string>) but it
// actually returns Seq.Set<string> (Seq<string,string>).
f(Seq.Set, "foo", "bar");

The fix takes the same form as well: remove the declaration. Since it's an
alias for Seq.Indexed.of, I've codemodded all callers to call that method
instead.

[szagi3891]

@samwgoldman
So can we expect this error to be corrected in the version "immutable@4.0.0-rc.3" ? :)

[samwgoldman]

@szagi3891 I don't think so. The fix is a single line: just remove the definition of Immutable.Seq.of from the flow type definitions. It might exist at runtime, but due to subtyping it's unsafe.

[tanem]

Hey @samwgoldman, just wanted to make sure I understood your suggestion ☝️

We're using 3.8.1, so it looks like we should ditch this line. But after you mentioned nothing would be done in 4.0.0-rc.3, does that mean something still might be done on the 3.8 branch?

Should we submit a PR / run with a fork / hang tight for a while?

[samwgoldman]

@tanem Right. It seems we have a local copy of the type definitions, so the change I made isn't going to sync out (we should keep these in sync, but alas).

I'm lazy, so I probably won't submit a PR, but feel free to use my explanation above. You're right on about removing that one line. It's a breaking change, but I'm not sure how this project deals with that.

[cristian-sima]

Same here. So the solution is to remove the line?

[philipp-spiess]

It seems we have a local copy of the type definitions, so the change I made isn't going to sync out (we should keep these in sync, but alas).

Any chance you can publish this as a third party definition in https://github.com/flowtype/flow-typed? I think this would make it a lot easier to work on flow types collaboratively (i.e for #1223).

[williamboman]

@szagi3891 I don't think so. The fix is a single line: just remove the definition of Immutable.Seq.of from the flow type definitions. It might exist at runtime, but due to subtyping it's unsafe.

What if you automatically pull in type definitions from node_modules/?

[duro]

Agreeing with @philipp-spiess here. Simply commenting out a line in node_modules seems like a pretty nasty work around, since every dev on the team will need to do this every time they download the project. Porting these flow types over to flow-typed will allow me to persist this to the repo more easily.

[ndbroadbent]

I agree with @philipp-spiess and @duro, it would be good to either port these types to flow-typed so that it's easier to fix them manually. But releasing a new version with the fix would be really nice.

[ndbroadbent]

I've forked and pushed a branch to remove this line. Feel free to use my fork to get rid of the flow errors.

If you're using yarn:

$ yarn add FormAPI/immutable-js#v3.8.1-flow-fix

Or add this line to your package.json:

"immutable": "FormAPI/immutable-js#v3.8.1-flow-fix"

[leebyron]

I think removing Seq.of is the right call, even though it's breaking. It's really just an alias for IndexedSeq.of and also a less commonly used API. Will include in the next release.

[williamboman]

@leebyron Will this be released in a 3.x release too?

[MichaelDeBoey]

@williamboman Since it's merged with master, I think it will be available in the next RC (v4.0.0-rc.3)

[leebyron]

This is released in the latest rc

[leebyron]

@williamboman unfortunately this is a breaking change, so incompatible with a patch release atop the 3.x releases

[fabiomcosta]

OK I could work around this issue without having to update to 4.0.0, which I can't do ATM.
Here are the instructions: https://gist.github.com/fabiomcosta/617ef69320a5539ab5cdc510b0b648af

[prewk]

@fabiomcosta Thanks, that solved my problem!