Flow: v0.70.0 promise typing updates may have broken many library type defs

[janie128]

There's possibly an issue with this update https://github.com/facebook/flow/commit/1ca197c103dd5d678f002efb0b5925ca5d728ed7#diff-f265d6a6b6b64d4d096fddcf7d620a76, updating the promise type defs. It does not seem compatible with existing library type definitions that have promises in them.

There is an issue reported in the flow-typed repo for bluebird type definitions broken: https://github.com/flowtype/flow-typed/issues/2112

We have observed similar errors in graphql (lots more of these in graphql):

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/graphql/execution/execute.js.flow:233:17

Cannot call data.then because:
 • Either function [1] is incompatible with null [2].
 • Or function [1] is incompatible with undefined [3].
 • Or function [1] is incompatible with null [4].
 • Or function [1] is incompatible with undefined [5].

        node_modules/graphql/execution/execute.js.flow
        230│   data: MaybePromise<ObjMap<mixed> | null>,
        231│ ) {
        232│   if (isPromise(data)) {
    [1] 233│     return data.then(resolved => buildResponse(context, resolved));
        234│   }
        235│   return context.errors.length === 0
        236│     ? { data }

        /private/tmp/flow/flowlib_39856dc8/core.js
 [2][3] 611│     then(onFulfill: null | void, onReject: null | void): Promise<R>;
        612│     then<U>(
 [4][5] 613│       onFulfill: null | void,

iterall:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ node_modules/iterall/index.js.flow:687:18

Cannot call Promise.resolve(...).then because:
 • Either function [1] is incompatible with null [2].
 • Or function [1] is incompatible with undefined [3].
 • Or function [1] is incompatible with null [4].
 • Or function [1] is incompatible with undefined [5].

        node_modules/iterall/index.js.flow
    [1] 681│       function next() {
        682│         asyncIterator
        683│           .next()
        684│           .then(function(step) {
        685│             if (!step.done) {
        686│               Promise.resolve(callback.call(thisArg, step.value, i++, source))
        687│                 .then(next)
        688│                 .catch(reject)
        689│             } else {
        690│               resolve()

        /private/tmp/flow/flowlib_39856dc8/core.js
 [2][3] 611│     then(onFulfill: null | void, onReject: null | void): Promise<R>;
        612│     then<U>(
 [4][5] 613│       onFulfill: null | void,

The pg type defs from flow-typed:

Error ┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈┈ flow-typed/npm/pg_v6.x.x.js:214:3

Cannot extend Promise [1] with Query because:
 • function type [2] is incompatible with null [3] in the first argument of property then.
 • function type [2] is incompatible with null [4] in the first argument of property then.
 • function type [5] is incompatible with null [6] in the second argument of property then.
 • function type [5] is incompatible with null [7] in the second argument of property then.
 • function type [8] is incompatible with null [9] in the first argument of property catch.

        flow-typed/npm/pg_v6.x.x.js
    [1] 105│     connect(cb?: PoolConnectCallback): Promise<PoolClient>;
           :
        211│    *
        212│    * ToDo: should find a better way.
        213│   */
        214│   declare class Query extends Promise<ResultSet> {
    [2] 215│     then<U>( onFulfill?: (value: ResultSet) => Promise<U> | U,
    [5] 216│       onReject?: (error: PG_ERROR) => Promise<U> | U
        217│     ): Promise<U>;
        218│     // Because then and catch return a Promise,
        219│     // .then.catch will lose catch's type information PG_ERROR.
    [8] 220│     catch<U>( onReject?: (error: PG_ERROR) => ?Promise<U> | U ): Promise<U>;
        221│
        222│     on :
        223│     ((event: 'row', listener: (row: Row, result: ResultBuilder) => void) => events$EventEmitter )&
        224│     ((event: 'end', listener: (result: ResultBuilder) => void) => events$EventEmitter )&
        225│     ((event: 'error', listener: (err: PG_ERROR) => void) => events$EventEmitter );
        226│   }
        227│
        228│   /*
        229│    * lib/client.js

        /private/tmp/flow/flowlib_440cfe4/core.js
 [3][6] 611│     then(onFulfill: null | void, onReject: null | void): Promise<R>;
        612│     then<U>(
    [4] 613│       onFulfill: null | void,
           :
    [7] 618│       onReject: null | void | ((error: any) => Promise<U> | U)
        619│     ): Promise<U>;
        620│
    [9] 621│     catch(onReject: null | void): Promise<R>;

There is likely a lot more of these but we just reverted to 0.69.0 and these errors will go away.

[alextes]

I'm confused. What's the issue 😅 ? Pointing out type errors is exactly what flow is built to do!

Flow since 0.70 is stricter about typing promises. As noted in the changelog that is likely to cause new type errors.

TL;DR: we should update bluebirds types, flow is working as expected 😄 .

[janie128]

Thanks for the response, @alextes

I guess the issue that I was trying to point out, was that this was a breaking change that broke a lot of things and led to quite some confusion on the user's end. It's great that flow is doing what it's supposed to do 😄 , but this pretty much meant that we cannot use 0.70 for now which is rather unfortunate. This puts users in an awkward situation where I would like to get bug fixes and any cool new updates in newer versions, but I don't have confidence letting npm automatically update to the latest minor version. Perhaps some semantic versioning would be helpful/nice :)

Since you guys are already aware of such a situation, then there's nothing else I wanted to point out. Please feel free to close the issue if there's nothing on flow's end to change. Thanks!

[alextes]

@janie128 ah I see, thanks for clarifying. In that case, check out npm's docs on semver. Specifically the following is relevant:

Allows changes that do not modify the left-most non-zero digit in the [major, minor, patch] tuple. In other words, this allows patch and minor updates for versions 1.0.0 and above, patch updates for versions 0.X >=0.1.0, and no updates for versions 0.0.X.

Many authors treat a 0.x version as if the x were the major "breaking-change" indicator.

Caret ranges are ideal when an author may make breaking changes between 0.2.4 and 0.3.0 releases, which is a common practice. However, it presumes that there will not be breaking changes between 0.2.4 and 0.2.5. It allows for changes that are presumed to be additive (but non-breaking), according to commonly observed practices.

^0.2.3 := >=0.2.3 <0.3.0

In other words, npm will not upgrade 0.69.x to 0.70.x . You now know that a <1.x.x minor upgrade is generally considered a breaking one 😄 . Although I fully agree things breaking is never fun, sometimes its for the best. This should at least clarify why the flow team might not consider the breakage a surprise, or problem.

[janie128]

@alextes Got it, thanks for the clarification :) We'll treat the 0.x minor upgrades to be possibly breaking and be careful.

[samwgoldman]

Sorry about the churn here. I hope it's not too painful to update Bluebird types. The updated types for Promise are more correct and catch a bunch of issues that the old types did not.