flow-typed Express's `Middleware` type is not correct

Express's `Middleware` type is not correct

Open HerringtonDarkholme opened this issue 7 years ago • 16 comments

  declare type Middleware =
    ((req: Request, res: Response, next: NextFunction) => mixed)|
    ((error: ?Error, req : Request, res: Response, next: NextFunction) => mixed);

https://github.com/flowtype/flow-typed/blob/master/definitions/npm/express_v4.x.x/flow_v0.25.x-/express_v4.x.x.js#L90

I don't know how flow handle union type of function. But according to the doc and variance rule, Middleware should be an intersection type, not union type.

Oct 13 '16 05:10 HerringtonDarkholme

Do you have an example of code that doesn't work? Union seems to be correct in this case

Oct 17 '16 20:10 vkurchatkin

I'm type checking vue-hackernews. https://github.com/vuejs/vue-hackernews-2.0/blob/master/server.js#L52

const express = require('express')
const app = express()
app.get('*', (req, res) => {
    return res.end('waiting for compilation... refresh in a moment.')
})

Oct 18 '16 01:10 HerringtonDarkholme

It should be an intersection type as far as I understand

Oct 18 '16 22:10 alexeygolev

I'm getting some odd errors with this, locally all is fine, but on CCI I get:

src/api/not-found.js:6
  6: const notFound: Middleware = (req, res, next) => {
                                  ^ arrow function. Could not decide which case to select
  6: const notFound: Middleware = (req, res, next) => {
                     ^^^^^^^^^^ union type
  Case 1 may work:
   94:     ((req: Request, res: Response, next: NextFunction) => mixed) |
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See lib: flow-typed/npm/express_v4.x.x.js:94
  But if it doesn't, case 2 looks promising too:
   95:     ((error: ?Error, req : Request, res: Response, next: NextFunction) => mixed);
            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ function type. See lib: flow-typed/npm/express_v4.x.x.js:95
  Please provide additional annotation(s) to determine whether case 1 works (or consider merging it with case 2):
    6: const notFound: Middleware = (req, res, next) => {
                                               ^^^^ parameter `next`
    6: const notFound: Middleware = (req, res, next) => {
                                     ^^^ parameter `req`
    6: const notFound: Middleware = (req, res, next) => {
                                          ^^^ parameter `res`

Oct 25 '16 09:10 Dakuan

Are you using the same flow version for both?

Oct 25 '16 09:10 alexeygolev

it looks that way

node_modules/.bin/flow version
Flow, a static type checker for JavaScript, version 0.33.0

Oct 25 '16 09:10 Dakuan

It's clearly something to do with versions, the older version had problems with union/intersection types

Oct 25 '16 09:10 alexeygolev

I'm seeing the same problem using flow 0.33.0 with the following usage:

// @flow

import type {
    Middleware
} from 'express';

const middleware: Middleware = (req, res, next) => {
    res.send('foo');
}

arrow function: /Users/rich/Projects/webpack-hot-server-middleware/src/index.js:7
Could not decide which case to select
union type: /Users/rich/Projects/webpack-hot-server-middleware/src/index.js:7
Case 1 may work:
function type: /Users/rich/Projects/webpack-hot-server-middleware/flow-typed/npm/express_v4.x.x.js:94
But if it doesn't, case 2 looks promising too:
function type: /Users/rich/Projects/webpack-hot-server-middleware/flow-typed/npm/express_v4.x.x.js:95
Please provide additional annotation(s) to determine whether case 1 works (or consider merging it with case 2):
parameter `next`: /Users/rich/Projects/webpack-hot-server-middleware/src/index.js:7
parameter `req`: /Users/rich/Projects/webpack-hot-server-middleware/src/index.js:7
parameter `res`: /Users/rich/Projects/webpack-hot-server-middleware/src/index.js:7

Has anybody found a solution to this?

Nov 12 '16 15:11 richardscarrott

The problem is that unless you specify types of the arguments, there is no way to know, which signature you want.

Nov 12 '16 15:11 vkurchatkin

@vkurchatkin Is there no way for flow to determine the types based on arity?

Nov 12 '16 15:11 richardscarrott

@richardscarrott Maybe express_v4.x.x.js#L91 need some one familiar with express to rewrite?

declare type express$Middleware =
  ((req: express$Request, res: express$Response, next: express$NextFunction) => mixed) |
  ((error: ?Error, req: express$Request, res: express$Response, next: express$NextFunction) => mixed);

Because it is used as a callback Function, even if you modified it to a Function Overloading(using &), it is still make a type union for every arguments, that is first argument must be express$Request|Error, and so on.