Express's Middleware type

[honzabrecka]

In my app, I'm using following error handler, which would break in case of err === null. Why is the error parameter allowed to be null in Middleware type?

export default function errorHandler(err, req, res, next) {
  const errorDetails = err.stack || err;
  ...
}

I would rather see something like this:

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

What do you think?

[jtlapp]

This may also explain the failure of the Express definitions to accept a 404 error handler:

app.use(function(req, res, next) {

FlowType gives me this error:

app.use(function(req, res, next) {
        ^ function. Could not decide which case to select

[jtlapp]

cc @bassettsj & @gcedo re Express libdef

[honzabrecka]

@jtlapp Flow can not decide which case to select because it ignores arities. You have to provide type of at least first different argument.

[jtlapp]

Thanks @honzabrecka. There appears to be some additional ambiguity in the following:

declare interface express$RouteMethodType<T> {
  (middleware: express$Middleware): T;
  (...middleware: Array<express$Middleware>): T;
  (path: string|RegExp|string[], ...middleware: Array<express$Middleware>): T;
}
declare interface express$RouterMethodType<T> {
  (middleware: express$Middleware): T;
  (...middleware: Array<express$Middleware>): T;
  (path: string|RegExp|string[], ...middleware: Array<express$Middleware>): T;
  (path: string, router: express$Router): T;
}

Revising them to the following would allow you to just specify the callback type, as well as prevent an empty parameter list from being valid:

declare interface express$RouteMethodType<T> {
  (middleware: express$Middleware, ...rest: Array<express$Middleware>): T;
  (path: string|RegExp|string[], ...middleware: Array<express$Middleware>): T;
}
declare interface express$RouterMethodType<T> {
  (middleware: express$Middleware, ...rest: Array<express$Middleware>): T;
  (path: string|RegExp|string[], ...middleware: Array<express$Middleware>): T;
  (path: string, router: express$Router): T;
}

My project adds session information to the request object and helper methods to both the request and response objects. To accommodate that, I had to extend the express request and response types with my own types and then write wrapper classes for the application and the router to accept signatures that us my extended request and response.

[jtlapp]

Also, in #454 I'm arguing for the Express definitions to employ ErrnoError.

[mwalkerwells]

@jtlapp I have the same dilemma—ideally we would not need to wrap the class, as mutating the request is common with middleware.

Posted Questions:

• https://github.com/flowtype/flow-typed/commit/fe8240e04355ce44d1cdb6bfb5766030a076d196#commitcomment-20036791
• http://stackoverflow.com/questions/40903212/express-middleware-mutate-request-type-flowtype

If you were to update the libdef, what would you change to avoid wrapping? I could not figure it out...

@bassettsj I'm curious if you have any ideas?

[jtlapp]

I think it would have to be possible to parameterize the libdef on a per-module basis -- that would mean a change to FlowType. I am quite pleased with me extensions of the Express request and response classes, because FlowType is enforcing my extensions.

[mwalkerwells]

@bassettsj @jtlapp My approach so far has been to have the express$Application polymorphic over the request & response type, allowing me to pass in types that extend both at instantiation. The problem with this is that it's important that you do not try to access the properties on the extended types before you add the corresponding Middleware.

[mwalkerwells]

possible to parameterize the libdef on a per-module basis

@jtlapp What would this look like in your opinion?

[jtlapp]

Maybe something like this to snag a type directly:

import type { express$Application<MyRequestType, MyResponseType> } from 'express';

It gets tricker loading modules. Maybe something like this:

var express = require('express')<MyRequestType, MyResponseType>;
var app = express();
var router = express.Router();

[jtlapp]

It would also be possible to do without the module-loading syntax:

import type { express$Application<MyRequestType, MyResponseType> } from 'express';

var express = ((require('express'): any): express$Application);
var app = express();
var router = express.Router();

If we need to name the particular parameterization:

import type { MyAppType = express$Application<MyRequestType, MyResponseType> } from 'express';

var express = ((require('express'): any): MyAppType);

But libdefs would need the option of assigning default parameter types.

[bassettsj]

Hmmm... this is a tough one. I think you could improve this by making sure to define that class as extending the base class and make sure that you add the type definitions for them to middleware functions:

// @flow
const app = express();
app.use(session(...));
declare class session$Request, extends express$Request {
    session: { [key: string]: mixed };
}

app.use((req: session$Request, res: express$Response, next: express$NextFunction): void => {
  req.session.user = { id: 2 };
  next();
});

I think we will probably have to add parameterization to the lib defs, but I am not sure what the best way to do this. We should look to see how definitely-typed accomplished this and see if we could follow their lead.

[mwalkerwells]

Ah, yes, similar to what I have been using.

I'm also adding session information to the request, but this is the problem I see with this approach:

import express from 'express'

// Referencing express$Request  & express$Response only for example. These classes do not exist in the global scope. 
class MyResponseType extends express$Response {}
class MyRequestType extends express$Request {
    session: string;
}

const app: express$Application<MyRequestType, MyResponseType> = express()

app.use('/', (req: MyRequestType, res: MyResponseType, next: express$NextFunction) => {
    console.log(req.sessoin) // ERROR: req has not been extended yet
    next()
})

app.use(session({}) /* extends req to be MyRequestType */ )

The order at which you add the middleware matters. Am I missing something?

[jtlapp]

Right @mwalkerwells. My solution doesn't allow for the type to change over time. FlowType wouldn't be able to protect me from using properties before I added them.

[jtlapp]

@bassettsj's solution looks good too, except that declarations are global, so libdefs could only be parameterized once across the entire application. Maybe that's desireable?

(Or are declarations local outside libdefs? I'm still new to FlowType...)

[mwalkerwells]

After a bit of reading, it seems Typescript uses a concept of "Declaration Merging": https://www.typescriptlang.org/docs/handbook/declaration-merging.html

https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/express-serve-static-core/express-serve-static-core.d.ts#L12

If I understand this correctly, applying this concept in flow means I should be able to do the following (doesn't work):

app.use(session({}) /* extends req to include session */ )

// Indicate that req now includes session. Order still matters, but a bit better...
declare class express$Request {
  session: { [key: string]: mixed }
}

app.use('/', (req: express$Request, res: express$Response, next: express$NextFunction) => {
    console.log(req.sessoin) // Ideally works, but doesn't
    next()
})

@bassettsj I do not believe your approach works, since that middleware is incompatible. Functions are contravariant over their arguments, meaning you could only use a function that has arguments that are subtypes (or invariant) over the original arguments.

[mwalkerwells]

@jeffmo @gcanti Thought either of you might know how to best model this. Without workarounds: A) statically typing a polymorphic express$Application at instantiation, where the order at which you add middleware is fragile, or B) without wrapping express$Application in a new class...

...how could we have the types ofreq: express$Request & req: express$Reponse evolve over type as new middleware is added?

[jtlapp]

Oh you're right @mwalkerwells. I missed that about @bassettsj's post. I thought he had written this, which is close to the TypeScript solution:

const app = express();
app.use(session(...));
declare class express$Request, extends express$Request {
    session: { [key: string]: mixed };
}

app.use((req: express$Request, res: express$Response, next: express$NextFunction): void => {

[mwalkerwells]

@bassettsj Arguably, one advantage to this diff is that the default "()" export is polymorphic, meaning that you can do the following:

1

const app = express()
app.use((req: express$Request, res: express$Response, next: express$NextFunction) => {
    next()
})

OR 2

const app = express()
app.use((req: MyRequest, res: MyResponse, next: express$NextFunction) => {
    console.log(req.myspecialvar)
    next9()
})

OR 3

const app: express$BaseApplication<MyRequest, MyResponse> = express()
app.use((req: MyRequest, res: MyResponse, next: express$NextFunction) => {
    console.log(req.myspecialvar)
    next9()
})

With this approach, the middleware type is defined by the intersection of all the middleware you pass to the express app...but with the option of statically bounding (3) if you want additional type safety.

[mwalkerwells]

@jtlapp How did you avoid this issue if you're extending the class? I just put up a pull request: https://github.com/flowtype/flow-typed/pull/508

[jtlapp]

I wrote wrapper classes for the Express application and the router, and I use those instead. My wrapper classes have to duplicate the method signatures. Actually, I'm not entirely happy with the current libdef (see above), so i also added my own embellishments. Unfortunately, my mods are pretty specific to my application, so it doesn't make sense to make it available for others to use too.

[mwalkerwells]

Sorry, I should have been more specific. By "this issue", I meant what I had in my diff: https://github.com/flowtype/flow-typed/pull/508#issue-193245089.

I actually really like this approach... :)

Thanks, I now understand your approach when you say "wrapper classes have to duplicate the method signatures."

[jtlapp]

I'll have to look at your #508 mod more closely. I've pulled out most of my app-specific stuff, so you can see how I did the wrapping:

// @flow

/******************************************************************************
Express request/response extensions and supporting wrapper classes.
******************************************************************************/

var express = require('express');

/*::
declare class _SessionRequest extends express$Request
{
    // custom extensions

    userInfo: {
        userID: number;
        displayName: string;
        privileges: string;
    };

    // request internals

    connection: {
        remoteAddress: string;
    };
    requestTimeUTC: number;
}

declare class _SessionResponse extends express$Response
{
    // custom extensions

    sessionState: any; // replace 'any' with your custom representation
}

// adapted from express$NextFunction
declare type _SessionNext = (err: ?ErrnoError) => void;

// adapted from express$Middleware
declare type _SessionMiddleware =
    ((req: _SessionRequest, res: _SessionResponse, next: _SessionNext)
        => mixed) |
    ((err: ErrnoError, req: _SessionRequest, res: _SessionResponse,
        next: _SessionNext) => mixed);

declare interface _SessionRouteMethodType<T>
{
    // adapted from express$RouteMethodType
    (middleware: _SessionMiddleware, ...extra: Array<_SessionMiddleware>): T;
    (path: string|RegExp|string[], ...middleware: Array<_SessionMiddleware>): T;
}

declare interface _SessionRouterMethodType<T>
{
    // adapted from express$RouterMethodType
    (middleware: _SessionMiddleware, ...extra: Array<_SessionMiddleware>): T;
    (path: string|RegExp|string[], ...middleware: Array<_SessionMiddleware>): T;
    (path: string, router: _SessionRouter): T;
}

declare class _SessionRouter
{
    // adapted from express$Route and express$Router
    constructor(options?: express$RouterOptions): void;
    all: _SessionRouteMethodType<this>;
    get: _SessionRouteMethodType<this>;
    post: _SessionRouteMethodType<this>;
    put: _SessionRouteMethodType<this>;
    head: _SessionRouteMethodType<this>;
    delete: _SessionRouteMethodType<this>;
    use: _SessionRouterMethodType<this>;
}

declare class _SessionApplication extends _SessionRouter
{
    // adapted from express$Application
    constructor(): void;
    locals: {[name: string]: mixed};
    mountpath: string;
    listen(port: number, hostname?: string, backlog?: number, callback?: _SessionNext): Server;
    listen(port: number, hostname?: string, callback?: _SessionNext): Server;
    listen(port: number, callback?: _SessionNext): Server;
    listen(path: string, callback?: _SessionNext): Server;
    listen(handle: Object, callback?: _SessionNext): Server;
    disable(name: string): void;
    disabled(name: string): boolean;
    enable(name: string): void;
    enabled(name: string): boolean;
    engine(name: string, callback: Function): void;
    // FlowType does not allow get() to be declared
    // get(name: string): mixed;
    set(name: string, value: mixed): mixed;
    render(name: string, optionsOrFunction: {[name: string]: mixed}, callback: express$RenderCallback): void;
}
*/

class RouterWrapper
{
    // largely calls router via apply(), so the type doesn't matter
    /*:: router: any; */

    constructor(options, router) {
        if (router)
            this.router = router;
        else
            this.router = express.Router(options);
    }

    use(/*arguments*/) {
        // Apps only call use() at initialization, so it's okay to have a
        // heavy implementation. Express requires its own router type.

        let args = Array.prototype.slice.call(arguments);
        for (let i = 0; i < args.length; ++i) {
            if (args[i] instanceof RouterWrapper)
                args[i] = args[i].router;
        }
        return this.router.use.apply(this.router, args);
    }

    get(/*arguments*/) {
        return this.router.get.apply(this.router, arguments);
    }

    post(/*arguments*/) {
        return this.router.post.apply(this.router, arguments);
    }

    put(/*arguments*/) {
        return this.router.put.apply(this.router, arguments);
    }

    delete(/*arguments*/) {
        return this.router.delete.apply(this.router, arguments);
    }
}

class AppWrapper extends RouterWrapper
{
    constructor() {
        super(null, express());
    }

    listen() {
        return this.router.listen.apply(this.router, arguments);
    }

    disable(name) {
        this.router.disable(name);
    }

    disabled(name) {
        return this.router.disabled(name);
    }

    enable(name) {
        this.router.enable(name);
    }

    enabled(name) {
        return this.router.enabled(name);
    }

    engine(name, callback) {
        this.router.engine(name, callback);
    }

    /*
    // TBD: add support for this additional signature. The base router uses
    // get() only as an HTTP method.
    get(name) {
        return this.router.get(name);
    }
    */

    set(name, value) {
        return this.router.set(name, value);
    }

    render(name, optionsOrFunction, callback) {
        this.router.render(name, optionsOrFunction, callback);
    }
}

//// EXPORTS //////////////////////////////////////////////////////////////////

/*::

// The $-prefixed variables correspond to their namesake Express objects.

export type $Application = _SessionApplication;
export type $Router = _SessionRouter;
export type $Request = _SessionRequest;
export type $Response = _SessionResponse;

// NextFunc defines the callback that Express "middleware" handlers use.

export type NextFunc = _SessionNext;

// StackHandler provides the usual signature for a "middleware" handler.

export type StackHandler =
        (req: $Request, res: $Response, next: NextFunc) => void;

// FormFields generically defines the value of req.body when populated by an HTML input field. Only use this when the input field names are dynamic. If the names can be hardcoded, instead define them as a specific type of object.

export type FormFields = { [key: string]: string };
*/

exports = module.exports = function () /*:$Application*/
{
    return ((new AppWrapper() /*:any*/) /*:$Application*/);
};

exports.static = function (root /*:string*/, options /*?:Object*/)
        /*:_SessionMiddleware*/
{
    return express.static(root, options);
}

exports.Router = function (options /*?:express$RouterOptions*/) /*:$Router*/
{
    return ((new RouterWrapper(options) /*:any*/) /*:$Router*/);
};

[mwalkerwells]

@jtlapp My diff was just merged & should reduce some of your code duplication, making typing the request/response mutation much easier.

https://github.com/flowtype/flow-typed/blob/master/definitions/npm/express_v4.x.x/flow_v0.28.x-/test_overrideUseMethodClassExtension.js

[rcocetta]

Shouldn't error in the middleware be of type any or mixed? https://expressjs.com/en/guide/error-handling.html