[express-serve-static-core] add RouteParameters type for IRouterMatcher that uses template literal types #51262

DetachHead · 2021-02-16T13:26:17Z

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.
Follow the advice from the readme.
Avoid common mistakes.
Run npm test <package to test>.

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: https://www.typescriptlang.org/docs/handbook/2/template-literal-types.html

…l types

typescript-bot · 2021-02-16T13:48:02Z

👋 Hi there! I’ve run some quick measurements against master and your PR. These metrics should help the humans reviewing this PR gauge whether it might negatively affect compile times or editor responsiveness for users who install these typings.

Let’s review the numbers, shall we?

Comparison details 📊

	master	#51262	diff
Batch compilation
Memory usage (MiB)	74.6	72.6	-2.7%
Type count	11115	11631	+5%
Assignability cache size	2413	2493	+3%

Language service
Samples taken	257	257	0%
Identifiers in tests	257	257	0%
`getCompletionsAtPosition`
Mean duration (ms)	370.8	369.8	-0.3%
Mean CV	11.2%	10.1%
Worst duration (ms)	628.7	689.9	+9.7%
Worst identifier	send	send
`getQuickInfoAtPosition`
Mean duration (ms)	394.4	397.9	+0.9%
Mean CV	10.5%	10.1%
Worst duration (ms)	704.8	706.4	+0.2%
Worst identifier	res	req

It looks like nothing changed too much. I won’t post performance data again unless it gets worse.

typescript-bot · 2021-02-16T14:15:58Z

@DetachHead Thank you for submitting this PR! I see this is your first time submitting to DefinitelyTyped 👋 — I'm the local bot who will help you through the process of getting things through.

This is a live comment which I will keep updated.

2 packages in this PR

express-serve-static-core — on npm, on unpkg
- Config files to check:
  - express-serve-static-core/ts4.0/tslint.json: not the expected form (check: rules)
express — on npm, on unpkg
- Config files to check:
  - express/ts4.0/tslint.json: not the expected form (check: rules)

Code Reviews

Because this is a widely-used package, a DT maintainer will need to review it before it can be merged.

You can test the changes in this PR in the Playground.

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
🕐 A DT maintainer needs to approve changes which affect more than one package

Once every item on this list is checked, I'll ask you for permission to merge and publish the changes.

Diagnostic Information: What the bot saw about this PR

{
  "type": "info",
  "now": "-",
  "pr_number": 51262,
  "author": "DetachHead",
  "headCommitOid": "57ab75649a406b6c891ef9c27a22a348267bc668",
  "lastPushDate": "2021-05-25T15:31:18.000Z",
  "lastActivityDate": "2021-05-25T16:26:28.000Z",
  "hasMergeConflict": false,
  "isFirstContribution": true,
  "tooManyFiles": false,
  "popularityLevel": "Critical",
  "pkgInfo": [
    {
      "name": "express-serve-static-core",
      "kind": "edit",
      "files": [
        {
          "path": "types/express-serve-static-core/express-serve-static-core-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/express-serve-static-core/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/express-serve-static-core/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/express-serve-static-core/ts4.0/express-serve-static-core-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/express-serve-static-core/ts4.0/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/express-serve-static-core/ts4.0/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/express-serve-static-core/ts4.0/tslint.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) (check: `rules`)"
        }
      ],
      "owners": [
        "borisyankov",
        "19majkel94",
        "kacepe",
        "micksatana",
        "samijaber",
        "JoseLion",
        "dwrss",
        "andoshin11"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    },
    {
      "name": "express",
      "kind": "edit",
      "files": [
        {
          "path": "types/express/express-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/express/package.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/express/ts4.0/express-tests.ts",
          "kind": "test"
        },
        {
          "path": "types/express/ts4.0/index.d.ts",
          "kind": "definition"
        },
        {
          "path": "types/express/ts4.0/tsconfig.json",
          "kind": "package-meta-ok"
        },
        {
          "path": "types/express/ts4.0/tslint.json",
          "kind": "package-meta",
          "suspect": "not [the expected form](https://github.com/DefinitelyTyped/DefinitelyTyped#user-content-linter-tslintjson) (check: `rules`)"
        }
      ],
      "owners": [
        "borisyankov",
        "CMUH",
        "puneetar",
        "dfrankland"
      ],
      "addedOwners": [],
      "deletedOwners": [],
      "popularityLevel": "Critical"
    }
  ],
  "reviews": [
    {
      "type": "stale",
      "reviewer": "danvk",
      "date": "2021-05-25T14:25:23.000Z",
      "abbrOid": "51f372c"
    },
    {
      "type": "stale",
      "reviewer": "robertvanhoesel",
      "date": "2021-05-25T12:17:12.000Z",
      "abbrOid": "59dc475"
    },
    {
      "type": "stale",
      "reviewer": "JoseLion",
      "date": "2021-05-13T01:05:28.000Z",
      "abbrOid": "59dc475"
    }
  ],
  "mainBotCommentID": 779863981,
  "ciResult": "pass"
}

typescript-bot · 2021-02-16T14:15:59Z

🔔 @borisyankov @19majkel94 @kacepe @micksatana @samijaber @JoseLion @dwrss @andoshin11 @CMUH @puneetar @dfrankland — 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.

…press which now requires >= 4.1

typescript-bot · 2021-02-16T14:38:58Z

@DetachHead The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

…kage instead

danger-public · 2021-02-16T15:19:17Z

Inspecting the JavaScript source for this package found some properties that are not in the .d.ts files.
The check for missing properties isn't always right, so take this list as advice, not a requirement.

express (`unpkg`)

was missing the following properties:

Route

Generated by 🚫 dangerJS against 59dc475

DetachHead · 2021-02-16T20:45:55Z

the failed job appears to be due to an issue in the environment where the benchmark is run:

sandersn · 2021-05-24T23:54:54Z

Welllll, this package is literally the main reason that template literal types exist, so I think we should merge this PR eventually. I'm going to merge it in about 16 hours, so that I have all day to watch for complaints. @DetachHead is that a good time for you? If not, I'll be prepared to revert the merge so you can fix any bugs at your leisure.

DetachHead · 2021-05-25T02:27:57Z

I'll probably be asleep as that's around midnight for me. But I'll check first thing in the morning

robertvanhoesel · 2021-05-25T10:43:50Z

types/express-serve-static-core/express-serve-static-core-tests.ts

+});
+
+// Default types - not using RouteParameters
+app.post('/' as string, (req, res) => {


This seems like a decent fallback workaround for non standard implementations. Would users still be able to provide custom parameters like in https://github.com/DefinitelyTyped/DefinitelyTyped/pull/51262/files#diff-c5d8f61506d81e402ccb7f5f031730ce5ee044b86777af13924d7edfae4243f1R45

yeah, this works:

app.get<{ foo: string; bar: number }>('/' as string, req => { req.params.foo; // $ExpectType string req.params.bar; // $ExpectType number req.params.baz; // $ExpectError });

i can add a test for this if you'd like

This is great, good to have this workaround on hand for when it might break someones implementation (although it is hard to imagine how it would). This is a great PR!

danvk

Overall very excited about this change! See comment about optional params in paths. I wouldn't want to block on that, but it would be worth seeing how hard it is to handle them correctly.

types/express-serve-static-core/index.d.ts

types/express-serve-static-core/ts4.0/express-serve-static-core-tests.ts

danvk · 2021-05-25T13:27:23Z

types/express-serve-static-core/ts4.0/express-serve-static-core-tests.ts

+    res.locals.foo; // $ExpectType boolean
+    res.locals.bar; // $ExpectError
+    res.send({ foo: 'ok' }); // $ExpectType Response<any, { foo: boolean; }, number>
+});


The Express docs have a few examples of tricky routes: https://expressjs.com/en/guide/routing.html#route-parameters

It would be a good idea to add them to the tests here to make sure they work as expected:

/flights/:from-:to

/plantae/:genus.:species

/user/:userId(\d+)

While it's not explicitly mentioned on that page, if you click through to path-to-regexp, it mentions that parameters can be optional, i.e. /path/:param? should make the type of req.params.param be string | undefined.

This may be tested elsewhere, but the path can also be a regular expression, and that should continue to work as before (there are no regex literals… yet!).

oh, wasn't aware of this. i'll add support for -, . and ?, though the regex will probably just have to be ignored

How about just adding a test for /path/:param? so that we know what the behavior is after this PR? Even if we know it's incorrect, it will be a good place to capture the context from this discussion and what more correct behavior would be in the future.

done. though i'm keen to rewrite the RouteParameters type to support this as well as properly ignore regex wrapped in parentheses. i think it'll need to be more like a lexer instead of just splitting on '/'|'-'|'.'. i'll get on that tomorrow

typescript-bot · 2021-05-25T13:58:52Z

@danvk, @robertvanhoesel, @JoseLion Thank you for reviewing this PR! The author has pushed new commits since your last review. Could you take another look and submit a fresh review?

typescript-bot · 2021-05-25T14:52:23Z

@DetachHead The CI build failed! Please review the logs for more information.

Once you've pushed the fixes, the build will automatically re-run. Thanks!

sandersn · 2021-05-25T15:51:33Z

@DetachHead you probably aren't still awake, but it looks like the PR is in a done state now, and the next chunk of work is a fairly large, separate thing. I'll merge this in a couple of hours in the off chance that you'll have a chance to object before then.

DetachHead · 2021-05-25T16:26:28Z

Yep all good to merge. Heading off to bed now

typescript-bot · 2021-05-25T17:01:48Z

I just published @types/[email protected] to npm.

typescript-bot · 2021-05-25T17:02:03Z

I just published @types/[email protected] to npm.

orta · 2021-05-26T07:11:34Z

I think the way that these changes were set up still allows TS versions < 4.0 to get the d.ts files for the template literals which breaks their builds - #53397

JensForstmann · 2021-05-27T08:48:24Z

For anybody having trouble right now with optional parameters and getting an error at build time:

server.get('/download/:entity/:name?', async (req, res, next) => {
	const entity = req.params.entity;

	// used to work in the past prior this PR
	const nameOr = req.params.name; // <<-- error here: Property 'name' does not exist on type 'RouteParameters<"/:entity/preview/:name?">'. Did you mean 'name?'?

	// as suggested
	const nameOr = req.params['name?']; // build will work, but variable will always be undefined
	
	// ...
});

The workaround that worked for me is changing the route to:

// Workaround: add empty string: route + ''
server.get('/download/:entity/:name?' + '', async (req, res, next) => {
	// ...
});

So just by concatenating two strings as a route it seems that the work put in to this PR will get disabled.

The type of req.params is then (property) Request<RouteParameters<string>, any, any, QueryString.ParsedQs, Record<string, any>>.params: RouteParameters<string>

DetachHead · 2021-05-27T08:54:05Z

you can do this:

server.get<{entity: string, name?: string}>('/download/:entity/:name?', async (req, res, next) => {
	// ...
});

that way you still get the type safety and aren't adding any unnecessary runtime code.

sorry about that, i'll try to get a PR ready with a fix for this within the next few days

DetachHead · 2021-05-31T10:38:24Z

#53513

angelsvirkov · 2022-05-31T10:47:38Z

@DetachHead is it really intended that projects using typescript versions below 4 should receive the type definitions from the ts4.0 folder?

Currently ,in the package.json of both @types/express and @types/express-serve-static-core the typesVersions property points to everything below <=4 instead of >=4 as normally done on type packages and defined in the TS Docs

    "typesVersions": {
        "<=4.0": { "*": ["ts4.0/*"] }
    }

DetachHead · 2022-05-31T11:24:13Z

@angelsvirkov that's intentional yeah. i followed the same pattern the rest of the repo seems to use. that's also how it's shown in the example here

@types/express-serve-static-core requires typescript version >= 4.1, see DefinitelyTyped/DefinitelyTyped#51262

add RouteParameters type for IRouterMatcher that uses template litera…

1422df0

…l types

DetachHead changed the title ~~add RouteParameters type for IRouterMatcher that uses template literal types~~ [express] add RouteParameters type for IRouterMatcher that uses template literal types Feb 16, 2021

DetachHead changed the title ~~[express] add RouteParameters type for IRouterMatcher that uses template literal types~~ [express-serve-static-core] add RouteParameters type for IRouterMatcher that uses template literal types Feb 16, 2021

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Feb 16, 2021

DetachHead added 4 commits February 17, 2021 00:07

specify minimum ts version for template literal types

70d4a7b

use RouteParameters type for IRoute & IRouterHandler

4864a0d

update tests

32f904a

fix $ExpectError's

e393fd3

DetachHead marked this pull request as ready for review February 16, 2021 14:15

DetachHead requested a review from borisyankov as a code owner February 16, 2021 14:15

typescript-bot added Critical package Edits multiple packages labels Feb 16, 2021

DetachHead added 4 commits February 17, 2021 00:21

set minimum ts version for absolute-url to 4.1 since it depends on ex…

87b3976

…press which now requires >= 4.1

fix lint errors

68f5fdb

update express test

7b078d7

set minimum typescript version for express-serve-static-core to 4.1

a865771

typescript-bot added the The CI failed When GH Actions fails label Feb 16, 2021

DetachHead added 2 commits February 17, 2021 00:54

revert changes to minimum ts version, gonna make a seperate <=4.0 pac…

02afe5c

…kage instead

backwards compatibility for ts <4.1

59dc475

typescript-bot added Check Config Changes a module config files and removed The CI failed When GH Actions fails labels Feb 16, 2021

typescript-bot added the Edits Infrastructure label Feb 17, 2021

DetachHead force-pushed the master branch from e5fb2f1 to 59dc475 Compare February 17, 2021 09:03

DetachHead mentioned this pull request Feb 17, 2021

fix failure due to local changes to package.json microsoft/DefinitelyTyped-tools#204

Merged

robertvanhoesel reviewed May 25, 2021

View reviewed changes

danvk reviewed May 25, 2021

View reviewed changes

DetachHead added 2 commits May 25, 2021 23:41

fix typos

bdbc6ae

better name for generic in IRoute

51f372c

typescript-bot removed the Unreviewed No one showed up to review this PR, so it'll be reviewed by a DT maintainer. label May 25, 2021

typescript-bot added the The CI failed When GH Actions fails label May 25, 2021

add test & TODO for optional params

57ab756

DetachHead force-pushed the master branch from e72cc47 to 57ab756 Compare May 25, 2021 15:31

typescript-bot removed the The CI failed When GH Actions fails label May 25, 2021

sandersn merged commit 95a309d into DefinitelyTyped:master May 25, 2021

orta mentioned this pull request May 26, 2021

@types/express-serve-static-core 4.17.20 causing build failures in @types/express #53397

Closed

5 tasks

DetachHead mentioned this pull request May 31, 2021

[express-serve-static-core] optional parameters and ./- delimited parameters #53513

Merged

8 tasks

mshabarov added a commit to vaadin/flow that referenced this pull request Sep 14, 2022

chore: Align TS version in flow-client with flow-server

0c32617

@types/express-serve-static-core requires typescript version >= 4.1, see DefinitelyTyped/DefinitelyTyped#51262

mshabarov mentioned this pull request Sep 14, 2022

chore: Align TS version in flow-client with flow-server vaadin/flow#14519

Merged

mshabarov added a commit to vaadin/flow that referenced this pull request Sep 14, 2022

chore: Align TS version in flow-client with flow-server (#14519)

b3eca0c

@types/express-serve-static-core requires typescript version >= 4.1, see DefinitelyTyped/DefinitelyTyped#51262

nwalters512 mentioned this pull request Jan 13, 2025

chore(deps): Deduplicate yarn.lock getsentry/sentry-javascript#14968

Merged

[express-serve-static-core] add RouteParameters type for IRouterMatcher that uses template literal types #51262

[express-serve-static-core] add RouteParameters type for IRouterMatcher that uses template literal types #51262

Uh oh!

Conversation

DetachHead commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 16, 2021 • edited by andrewbranch Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

2 packages in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Feb 16, 2021

Uh oh!

danger-public commented Feb 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

express (unpkg)

Uh oh!

DetachHead commented Feb 16, 2021

Uh oh!

sandersn commented May 24, 2021

Uh oh!

DetachHead commented May 25, 2021

Uh oh!

robertvanhoesel May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DetachHead May 25, 2021

Choose a reason for hiding this comment

Uh oh!

robertvanhoesel May 25, 2021

Choose a reason for hiding this comment

Uh oh!

danvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

danvk May 25, 2021

Choose a reason for hiding this comment

Uh oh!

DetachHead May 25, 2021

Choose a reason for hiding this comment

Uh oh!

danvk May 25, 2021

Choose a reason for hiding this comment

Uh oh!

DetachHead May 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DetachHead May 31, 2021

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented May 25, 2021

Uh oh!

typescript-bot commented May 25, 2021

Uh oh!

sandersn commented May 25, 2021

Uh oh!

DetachHead commented May 25, 2021

Uh oh!

typescript-bot commented May 25, 2021

Uh oh!

typescript-bot commented May 25, 2021

Uh oh!

orta commented May 26, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JensForstmann commented May 27, 2021

Uh oh!

DetachHead commented May 27, 2021

DetachHead commented Feb 16, 2021 •

edited

Loading

typescript-bot commented Feb 16, 2021 •

edited by andrewbranch

Loading

typescript-bot commented Feb 16, 2021 •

edited

Loading

typescript-bot commented Feb 16, 2021 •

edited

Loading

danger-public commented Feb 16, 2021 •

edited

Loading

express (`unpkg`)

robertvanhoesel May 25, 2021 •

edited

Loading

DetachHead May 25, 2021 •

edited

Loading

orta commented May 26, 2021 •

edited

Loading

angelsvirkov commented May 31, 2022 •

edited

Loading