[express]: adding generics for query type

[puneetar]

express-serve-static-core has the Request.query type as any. This pull request will make the type generic and allow the caller to define the expected type.

Migration guide:
#43434 (comment)

Please fill in this template.

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

Select one of these and delete the others:

If changing an existing definition:

• Provide a URL to documentation or source code which provides context for the suggested changes: <>
• If this PR brings the type definitions up to date with a new version of the JS library, update the version number in the header.
• If you are making substantial changes, consider adding a tslint.json containing { "extends": "dtslint/dt.json" }. If for reason the any rule need to be disabled, disable it for that line using // tslint:disable-next-line [ruleName] and not for whole package so that the need for disabling can be reviewed.

[typescript-bot]

@puneetar Thank you for submitting this PR!

🔔 @DeadAlready @borisyankov @CMUH @dfrankland @AmirTugi @19majkel94 @kacepe @micksatana @samijaber @aereal @JoseLion @dwrss @andoshin11 @thde @jindev @stbychkov - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[OliverJAsh]

Can we default this to unknown or PoorMansUnknown (see my PR #43427)?

[OliverJAsh]

In fact this should default to the return type of qs.parse, since that's the default query parser. https://expressjs.com/en/api.html#app.settings.table

qs.parse's return type is any 😢. How about:

type ParsedQs = { [key: string]: string | ParsedQs };

#43440

[puneetar]

As this is generic now, one could pass on the expected type. I think most of the times we know that query parameters to expect, and having it as { [key: string]: string | ParsedQs } would would not allow more specific types.

Also, have it as generic, would allow you to pass on { [key: string]: string | ParsedQs } as the parameter type.

[puneetar]

Ah, I see you mean to update the default value to type ParsedQs = { [key: string]: string | ParsedQs };. I think that should be a fine assumption to make.

[puneetar]

But that would require a dependency on ParsedQs Should that be the case?

[puneetar]

I would prefer to do this update in a separate PR. This may cause multiple other libraries to be updated.

[puneetar]

Also, from the examples for qs it seems it can create array of objects as well

var arraysOfObjects = qs.parse('a[][b]=c');
assert.deepEqual(arraysOfObjects, { a: [{ b: 'c' }] });

Probably it should be

type ParsedQs = { [key: string]: string | string[] | ParsedQs | ParsedQs[] };

[OliverJAsh]

This may cause multiple other libraries to be updated.

It didn't in my PR, so it should be fine.

[puneetar]

Updating the default value to be anything other than any would cause the libraries to break and would require an update to code to use type refinement. I don't think we should update the default type.

(req: Request , res: Response) => req.query.foo // Type: string | Query | string[] | Query[]

Now to use the foo query type I will have to update all the places, and use type refinement / type assertion to get rid of the type error.

Probably should be done as part of a major release. I don't want to take that on for now.

[OliverJAsh]

Probably should be done as part of a major release. I don't want to take that on for now.

I understand your concern about breaking other people's projects or libraries. However, whilst I do like the idea of waiting for a new major version of the library before making breaking changes to the types (to reduce inconvenience), in my experience that just isn't practical.

DefinitelyTyped has no system in place to allow us to coordinate breaking changes so that they are all released together. How would we remember to make this change when updating the types to support the next major version of Express? Is there even a new major version on the horizon? I don't think there's any way to coordinate these things. If we forget to make the change then, we'll have to wait for another major version to come along, which could take a very long time.

Therefore, I really do believe it's a choice between

• leaving the types broken (broken because they use any which completely disables type checking, thereby defeating the whole point of TypeScript), or
• fixing the types immediately.

I've been here many times before… 😒 FWIW, req.params used to be any, but we managed to fix that!

[OliverJAsh]

I would like us to resolve this discussion before this is merged: #43434 (comment)

[puneetar]

@OliverJAsh I pushed the update to the proper default type for query.

[OliverJAsh]

Maybe add a comment above here to say it's the return type of qs.parse, the default query parser (https://expressjs.com/en/api.html#app-settings-property).

[puneetar]

Hey @OliverJAsh, I just realized that the dictionary types don't take account when the key is not present. Should we update this to

export interface Query { [key: string]: string | string[] | Query | Query[] | undefined; }

[OliverJAsh]

In my opinion that's a bug in TypeScript. I don't believe we should provide a workaround here, since it applies to all index signatures (e.g. arrays). If the user wants to workaround that, they can use a helper instead of indexing directly: microsoft/TypeScript#13778 (comment)

[pscanf]

Not including undefined though breaks this usage pattern for typing the request object of a specific route, which I had found quite useful:

interface MyRouteRequest extends Request {
  // Validation of the query object is in place so I'm basically asserting the type
  query: {
    requiredParam: string;
    optionalParam?: string;
  }
}

With query: any this didn't raise any compilation error, with query: Query (or query: ParsedQs, like it is now) this raises the following error, caused by the ? of optionalParam:

Type 'undefined' is not assignable to type 'string | ParsedQs | string[] | ParsedQs[]'

I agree that the underlying problem is TypeScript's, though practically this change means that if before I could meaningfully and correctly type my query objects, now I can only go for a partially correct typing.

So I would suggest either adding undefined to the ParsedQs union type, or maybe better yet go back to using any. Rationale:

• as written above, it gives the developer the possibility to override the type
• ParsedQs is very broad as a type, so it doesn't give many more guarantees over any
• ParsedQs gives false guarantees about nullability (again, I agree it's a TypeScript problem, but at that point I would argue in favour of using any, which seems less deceptive)
• though qs is the default express query parser, express can use node's native query parser querystring, for which the ParsedQs typing would be incorrect

What do you think @puneetar , @OliverJAsh ? (If it sounds reasonable I'll open a PR for the change)

[OliverJAsh]

Not including undefined though breaks this usage pattern for typing the request object of a specific route

Could you switch from extending Request to passing the custom query type as a generic? This PR added the query as a generic to support this.

I understand though that this does mean you'll need to switch from your current approach. If someone wants to raise a PR to add undefined to the union I would be happy to approve it.

better yet go back to using any

I do not think this is a good idea. any completely disables type checking—you have to opt-in by providing a generic, otherwise you may get runtime exceptions.

app.get('/', req => {
  // no compile error, but throws at runtime!
  req.query.i.do.not.exist
});

As soon as any is used (whether it's intentional or not), it may leak into other places and cause all sorts of bugs.

const fn = (foo: string) => foo.length;
app.get('/', req => {
  // no compile error, but throws at runtime if `foo` does not exist!
  fn(req.query.foo);
});

With the current model, type safety is built-in. If you don't care about type safety, you can opt-out.

though qs is the default express query parser, express can use node's native query parser querystring, for which the ParsedQs typing would be incorrect

In that case you would use the generic to customise the query type to use querystring.ParsedUrlQuery instead. Alternatively you could write a function that asserts the type, then use that instead of reading req.query directly. That's actually what I'm doing:

/**
 * By default, Express uses `qs` to parse query strings:
 * https://expressjs.com/en/api.html#app.settings.table.
 *
 * However, we have switched this to use Node's built-in `querystring` parser instead. For this
 * reason, we need to assert the type.
 *
 * It is possible to change the type of `req.query` via a generic, however the generic is very
 * awkward to use since it requires many other unrelated generics to be provided at the same time.
 * */
export const getQueryForRequest = (req: express.Request) => req.query as ParsedUrlQuery;

[pscanf]

Could you switch from extending Request to passing the custom query type as a generic? This PR added the query as a generic to support this.

Yeah, sorry, did not specify that which is the obvious answer. In theory it's possible, but the code I'm working with is a bit more convoluted, in that there are several layers of interface extension, each adding some custom properties to the request object. Something like:

interface ILoggedRequest extends Request {}
interface ITracedRequest extends ILoggedRequest {}
interface IAuthenticatedRequest extends ITracedRequest {}
interface MyCustomRequest extends IAuthenticatedRequest {}

So I'd need to add generics to each step of the chain, some of which are defined in shared libraries, which makes it very complicated. Ofc it's not this package definition's problem, but if the proposed change makes sense for other use cases all the better.

I opened this PR #44645 for it.

[OliverJAsh]

There is going to be some resistance to this change, but nonetheless I do believe it's the right thing to do.

For future readers who come here after their builds have broken: the default req.query type has changed from any to Query.

Query is the return type of qs.parse, which is Express' default query parser.

interface Query {
  [key: string]: string | string[] | Query | Query[];
}

Why did we switch from any to Query? Because any completely disables type checking:

req.query.i.do.not.exist; // no compile error, but throws at runtime (!!)

Hopefully it's obvious why that's bad. It might be what you want (in which case you can use an assertion to any as an escape hatch), but it shouldn't be the default for everyone.

Why did we make this change now? There is no good time to make this change.

Why didn't we bump the major version? Because the version of the types must match the version of the library. #25677 (comment)

Migration guide

If you have code like this:

app.get("/:foo", req => {
  // $ExpectType any
  const q = req.query.q;
});

You should now use validation to refine the type:

app.get("/:foo", req => {
  // $ExpectType string | undefined
  const q = typeof req.query.q === "string" ? req.query.q : undefined;
});

This is more code, but it's safer. We can't just assume q is going to be a string—it could be string[] or something else—so we have to validate it.

Alternatively, if you don't want to validate req.query but you do want to define a custom type, you can use the new generic added in this PR:

// Query can be a custom type
type MyQuery = { q: string };
app.get<{}, any, any, MyQuery>("/:foo", req => {
  req.query.q; // $ExpectType string
  req.query.a; // $ExpectError
});

This new generic may also be useful if you've defined a custom query parser.

If you want to revert to the previous behaviour, i.e. opt-out of type safety, you can cast req.query to any, or just don't upgrade the types (either by using lockfiles, pinning the version in your package.json, or using package resolutions).

app.get("/:foo", req => {
  // $ExpectType any
  (req.query as any).q;
});

/cc @puneetar You may want to link to this migration guide from the top of this PR, so it's easier to find.

[typescript-bot]

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

[RyanCavanaugh]

@OliverJAsh this looks like a sign-off but I'd like you to confirm

[OliverJAsh]

I'm not sure I should be the one to sign this off, since I inspired this PR in the first place (with #43427).

@RyanCavanaugh Whilst you're here, it would be good to sanity check with you that this is the right thing to do (migrating away from any, for a very popular library). As I mentioned, I'm not sure there is a good time to make this change.

[SergeyCherman]

@jheredia if it kills your pipelines feel free to use the fix I had above, ie add @types/express-serve-static-core@4.17.3 to your dependencies (by specifying to 4.17.3 version you won't have the update that made the recent changes.)

[OliverJAsh]

@jheredia Can you provide a reduced test case in the form of a repo, so I can check it out and see what's going on? I don't fully understand the issue from your description.

[jheredia]

@SergeyCherman thanks, we're trying that but it seems that the yarn.lock is giving some issues, maybe removing that and downgrading the dependency we can solve this.

@OliverJAsh I'll try to do that.

Basically the point is that we can't send a custom query type (Or maybe I'm doing it wrong) as the express router won't allow the express-validator ValidationChain to be sent (Once again, maybe I'm setting the code incorrectly)

Also using destructuring as shown on my previous comment creates issues with this solution

const q = typeof req.query.q === "string" ? req.query.q : undefined;

I know that this PR was merged to increase type safety but it seems weird that in order to reap the benefits we should revert to ES5, as I don't find an easy way to use the object destructuring with the types definition.

Thanks!

[OliverJAsh]

@jheredia This should work.

type CustomQuery = {
    a: string;
    b: string;
    c: string;
    d: string;
    e: string;
};

const someOtherHandler: express.RequestHandler = (req, res, next) => next();

router.get<{}, any, any , CustomQuery>('/:foo', someOtherHandler, (req) => {
    const { query: { a, b, c, d, e }, query } = req;
    query; // $ExpectType CustomQuery
    a; // $ExpectType string
    b; // $ExpectType string
    c; // $ExpectType string
    d; // $ExpectType string
    e; // $ExpectType string
});

Note: if you switch CustomQuery to using an interface instead of a type alias, this will error with "no overload matches this call." I'm not sure why this only happens with interfaces, but if you can use a type alias I would suggest doing that. Otherwise, I was able to workaround that by making sure that every request handler was annotated with the query:

interface CustomQuery {
    a: string;
    b: string;
    c: string;
    d: string;
    e: string;
}

const someOtherHandler: express.RequestHandler<{}, any, any , CustomQuery> = (req, res, next) => next();

router.get<{}, any, any , CustomQuery>('/:foo', someOtherHandler, (req) => {
    const { query: { a, b, c, d, e }, query } = req;
    query; // $ExpectType CustomQuery
    a; // $ExpectType string
    b; // $ExpectType string
    c; // $ExpectType string
    d; // $ExpectType string
    e; // $ExpectType string
});

Note this workaround will only work after #43764.

Note also that using the generic is essentially bypassing type safety—it would be better to validate req.query using a user-defined type guard:

declare const getCustomQuery: (query: express.Query) => query is CustomQuery;

const customQuery = getCustomQuery(req.query)
const { a, b, c, d, e } = customQuery;

[jheredia]

Thanks @OliverJAsh we'll try that and let you know if worked :)

[monsonjeremy]

@OliverJAsh This change makes sense and I appreciate the write-up.
I'm a little confused however as to how I could apply this change into my own setup>

I've basically created a custom Request type like so

import { Request } from 'express'
export interface CustomReq extends Request {
  someCustomReqProperty: number;
}

I usually import this for use in my handlers like so:

app.get('/', (req: CustomReq, res: Response) => {})

Do you have any suggestion on how I could use you changes to create a custom query type, something like:

import { Query, Request } from 'express'

export type CustomQuery = { someQueryParam: string } & Query
export interface CustomReq extends Request {
  someCustomReqProperty: number;
  query: CustomQuery;
}

[OliverJAsh]

@monsonjeremy Hmm, this seems to work:

import * as express from 'express';
import { Request, Response } from 'express';
import { Query } from 'express-serve-static-core';
export type CustomQuery = { someQueryParam: string } & Query;
export interface CustomReq extends Request {
    someCustomReqProperty: number;
    query: CustomQuery;
}
const app = express();
app.get('/', (req: CustomReq, res: Response) => {
    req.query.someQueryParam; // $ExpectType string
});

package.json:

{
  "dependencies": {
    "@types/express": "4.17.6",
    "@types/express-serve-static-core": "4.17.5",
    "typescript": "3.8.3"
  }
}

Note it does error when strictFunctionTypes is enabled, but I don't think this is a new problem: I can reproduce that using your previous code, running against the Express versions prior to this change:

import * as express from 'express';
import { Request, Response } from 'express';
export interface CustomReq extends Request {
    someCustomReqProperty: number;
}
const app = express();
// Error
// No overload matches this call.
app.get('/', (req: CustomReq, res: Response) => {});

package.json:

{
  "dependencies": {
    "@types/express": "4.17.5",
    "@types/express-serve-static-core": "4.17.3",
    "typescript": "3.8.3"
  }
}

[monsonjeremy]

@OliverJAsh Awesome, thanks so much for the insightful response. Oddly enough I had actually tried this before, but it seems that there is some issues when the express-serve-static-core library gets installed (which I installed to prevent an ESLint error) since that library's types are outdated.

[kpping]

Hi, I have a question

If we change query to [key: string]: string | Query | Array<string | Query> type while lib Express.js doesn't guarantee value (It can be undefined if it doesn't present unlike params) Is it really safer than any type ? Because it introduced new missing leading problem.

And in my opinion to prevent breaking change in many people code base, the default value should be any.

[stolzda]

Not going to question if this change was good or not. However, according to
https://semver.org/ this should not have been done in a Patch release - because all packages which depend on this package and are included only indirectly are now broken, or need manual fixing...

[OliverJAsh]

@stolzda

Why didn't we bump the major version? Because the version of the types must match the version of the library. #25677 (comment)

#43434 (comment)

[thw0rted]

I'm just getting to this party half a year late. The migration guide didn't address this, exactly, so maybe somebody who's still watching this PR will have some insight. I had code like

app.get("/", (req, res) => {
  if (Object.keys(req.query).length > 0) {
    delete req.query.keyIDon'tLike;
    return res.redirect(`./login?${querystring.stringify(req.query)}`);
  }
  // ...
});

After updating my express types, I started to get an error that the type of req.query was not compatible with the function signature for querystring.stringify. Reading through the discussion here, and the express docs, it looks like the default settings (extended=true) use qs not querystring as the parser. I didn't change the defaults. I think it works fine if I just switch from querystring.stringify to qs.stringify. Is that the right course of action? If I want to use the built-in Node function, does it make sense to just cast it with as Record<string,string> or similar? (Are the two stringify functions even substantially different?)

[OliverJAsh]

switch from querystring.stringify to qs.stringify.

That's one solution. Alternatively you can use the generic:

import { ParsedUrlQuery } from 'querystring';

app.get<{}, any, any, ParsedUrlQuery>("/", (req, res) => {
  if (Object.keys(req.query).length > 0) {
    delete req.query.keyIDon'tLike;
    return res.redirect(`./login?${querystring.stringify(req.query)}`);
  }
});

… or an assertion:

import { ParsedUrlQuery } from 'querystring';

app.get("/", (req, res) => {
  if (Object.keys(req.query).length > 0) {
    delete req.query.keyIDon'tLike;
    return res.redirect(`./login?${querystring.stringify(req.query as ParsedUrlQuery)}`);
  }
});

Are the two stringify functions even substantially different?

IIRC qs has better support for nesting.

[vicasas]

We use ES6+ and have run into the query destructuring problem.

Would doing this be correct?

interface IRequest {
  a: string
  b: string
  c: string
}

type QueryString = { a: string; b: string }

class MyMSController {
  async myMSController(req: Request, res: Response): Promise<void> {
    const { a, b } = req.query as QueryString
    const objQuery: IRequest = { a, b, c: '' }

    console.log(objQuery)
  }
}