fix(serve-static): make response generic by devanshj · Pull Request #48591 · DefinitelyTyped/DefinitelyTyped

devanshj · 2020-10-07T18:14:45Z

Right now the types are as if the library is coupled to express, which is not the case as the readme shows an example of it running with node server and nowhere in the source code it uses express-specific methods on the response or request.

So because the types of request and response are that of express (which is not needed) the example from the readme doesn't compile. So to fix it, I've made the types adhere to the implementation ie the request and response can be anything as long as they extend http.IncomingMessage and http.OutgoingMessage respectively. Response needs to be generic as it's used in setHeaders option.

So it indeed is a breaking change but only for people who invoke the serveStatic with setHeaders option using express apis and the invocation is not directly passed to use (which makes the response not infer to express.Response). The first commit adds test case to indicate breaking change.

typescript-bot · 2020-10-07T18:15:20Z

@devanshj Thank you for submitting this PR!

This is a live comment which I will keep updated.

1 package in this PR

serve-static on npm, on unpkg

Code Reviews

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

Status

✅ No merge conflicts
✅ Continuous integration tests have passed
❌ Most recent commit is approved by DT maintainers

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": 48591,
  "author": "devanshj",
  "owners": [
    "urossmolnik",
    "LinusU"
  ],
  "dangerLevel": "ScopedAndTested",
  "headCommitAbbrOid": "fbb12c7",
  "headCommitOid": "fbb12c7e0c34275dbea1a4bdb62dc50fd24b0dd3",
  "mergeIsRequested": false,
  "stalenessInDays": 2,
  "lastPushDate": "2020-10-10T15:47:21.000Z",
  "lastCommentDate": "2020-10-16T17:01:59.000Z",
  "maintainerBlessed": false,
  "reviewLink": "https://github.com/DefinitelyTyped/DefinitelyTyped/pull/48591/files",
  "hasMergeConflict": false,
  "authorIsOwner": false,
  "isFirstContribution": false,
  "popularityLevel": "Critical",
  "newPackages": [],
  "packages": [
    "serve-static"
  ],
  "files": [
    {
      "path": "types/serve-static/index.d.ts",
      "kind": "definition",
      "package": "serve-static"
    },
    {
      "path": "types/serve-static/serve-static-tests.ts",
      "kind": "test",
      "package": "serve-static"
    }
  ],
  "hasDismissedReview": false,
  "ciResult": "pass",
  "reviewersWithStaleReviews": [
    {
      "reviewedAbbrOid": "dcb021a",
      "reviewer": "rbuckton",
      "date": "2020-10-10T00:53:22Z"
    }
  ],
  "approvalFlags": 0,
  "isChangesRequested": false
}

typescript-bot · 2020-10-07T18:15:21Z

🔔 @urossmolnik @LinusU — 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.

devanshj · 2020-10-07T18:15:41Z

types/serve-static/index.d.ts

+    interface RequestHandler<R extends http.OutgoingMessage> {
+        (request: http.IncomingMessage, response: R, next: () => void): any;
    }
-    function serveStatic(root: string, options?: ServeStaticOptions): express.Handler;


I'm not sure if this is required or what, seems it's not so I removed it as it looks like a duplicate declaration.

I think it may have been added to (incorrectly) model --esModuleInterop. serveStatic doesn't assign itself as a property of itself, so removing this is correct.

Yeah exactly I realized that later and was about to point that out as I myself had hit a runtime error because import { serveStatic } from "serve-static" compiled anyway.

devanshj · 2020-10-07T18:17:33Z

types/serve-static/index.d.ts

 declare namespace serveStatic {
    var mime: typeof m;
-    interface ServeStaticOptions {
+    interface ServeStaticOptions<R extends http.OutgoingMessage = http.OutgoingMessage> {


Making the type parameter optional to avoid unnecessary breaking change for those who have used ServeStaticOptions in their codebase. I often avoid going so overboard to avoid breaking changes in types but it's not that bad hence went with it.

Isn't this still a breaking change? Anyone who had previously used serveStatic with a custom setHeaders callback will now get the weaker http.OutgoingMessage instead of express.Response when not contextually typed. This is mitigated somewhat by the fact that app.use(serveStatic(...)) should result in R being contextually typed to express.Response, but if anyone writes code like this then they will be broken:

const middleware = serveStatic(...); // no contextual type app.use(middleware); // error: `OutgoingMessage` is not assignable to `Response`.

It would be good to have the tests verify the contextual type for the app.use(serveStatic(...)) case. I'm not sure how often anyone writes serveStatic(...) outside of a .use(), so I'm not sure how much of an impact this change will be.

Well ultimately it is a breaking change although making type parameter optional saves some error in cases where users have written const options: ServeStaticOptions = { ... } and are not using express exclusive methods on the response.

[...] but if anyone writes code like this then they will be broken:

const middleware = serveStatic(...); // no contextual type app.use(middleware); // error: `OutgoingMessage` is not assignable to `Response`.

Umm, not really? This would compile ...

const middleware = serveStatic({ setHeaders: res => res.setHeader("foo, bar") }) // R is http.OutgoingMessage app.use(middleware) // works because `use` will invoke with `express.Response` which is wider than `http.OutgoingMessage` which is required by `middleware`

... however this would not (which would have compiled with previous version) ...

const middleware = serveStatic({ setHeaders: res => res.set("foo, bar") }) // R is http.OutgoingMessage, error: property set does not exist app.use(middleware) // no error here

It would be good to have the tests verify the contextual type for the app.use(serveStatic(...)) case.

Yes as you might have seen I have added tests for this and the above expected error as well.

I'm not sure how often anyone writes serveStatic(...) outside of a .use(), so I'm not sure how much of an impact this change will be.

Yeah and even if they do write serveStatic(...) outside use it's won't be a breaking change as long as they don't use express exclusive methods in setHeaders. As I had said in the description:

So it indeed is a breaking change but only for people who invoke the serveStatic with setHeaders option using express apis and the invocation is not directly passed to use (which makes the response not infer to express.Response).

But all things said I don't think this will have a huge impact.

devanshj · 2020-10-07T18:21:55Z

types/serve-static/index.d.ts

-
-import * as express from "express-serve-static-core";
 import * as m from "mime";
+import * as http from "http";


This assumes users have @types/node installed, which I guess is okay?

You should probably indicate that with /// <reference types="node" />.

devanshj · 2020-10-07T18:22:40Z

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

    maxAge: 0,
    redirect: true,
-    setHeaders: function(res: express.Response, path: string, stat: any) {
+    setHeaders: function(res, path: string, stat: any) {


(does work with : express.Response too ofc)

Please add an // $ExpectType on the next line to verify the contextual type is still express.Response.

typescript-bot · 2020-10-07T18:23:38Z

👋 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	#48591	diff
Batch compilation
Memory usage (MiB)	67.0	76.5	+14.2%
Type count	9994	9996	0%
Assignability cache size	1798	1777	-1%

Language service
Samples taken	36	54	+50%
Identifiers in tests	36	54	+50%
`getCompletionsAtPosition`
Mean duration (ms)	483.3	465.2	-3.7%
Mean CV	12.9%	11.7%
Worst duration (ms)	609.4	577.2	-5.3%
Worst identifier	mime	set
`getQuickInfoAtPosition`
Mean duration (ms)	475.6	471.8	-0.8%
Mean CV	12.4%	12.0%	-3.0%
Worst duration (ms)	563.3	554.0	-1.7%
Worst identifier	express	index

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

rbuckton · 2020-10-10T00:54:33Z

types/serve-static/index.d.ts

-
-import * as express from "express-serve-static-core";
 import * as m from "mime";
+import * as http from "http";


You should probably indicate that with /// <reference types="node" />.

rbuckton · 2020-10-10T01:01:58Z

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

    maxAge: 0,
    redirect: true,
-    setHeaders: function(res: express.Response, path: string, stat: any) {
+    setHeaders: function(res, path: string, stat: any) {


Please add an // $ExpectType on the next line to verify the contextual type is still express.Response.

rbuckton · 2020-10-10T01:02:20Z

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

+
+app.use(serveStatic('/infers-express-response-when-passed-to-express-use', {
+    setHeaders: function(res) {
+        res.set('foo', 'bar');


Please add an // $ExpectType on the next line to verify the contextual type is still express.Response.

rbuckton · 2020-10-10T01:03:26Z

types/serve-static/index.d.ts

 declare namespace serveStatic {
    var mime: typeof m;
-    interface ServeStaticOptions {
+    interface ServeStaticOptions<R extends http.OutgoingMessage = http.OutgoingMessage> {


Isn't this still a breaking change? Anyone who had previously used serveStatic with a custom setHeaders callback will now get the weaker http.OutgoingMessage instead of express.Response when not contextually typed. This is mitigated somewhat by the fact that app.use(serveStatic(...)) should result in R being contextually typed to express.Response, but if anyone writes code like this then they will be broken:

const middleware = serveStatic(...); // no contextual type app.use(middleware); // error: `OutgoingMessage` is not assignable to `Response`.

It would be good to have the tests verify the contextual type for the app.use(serveStatic(...)) case. I'm not sure how often anyone writes serveStatic(...) outside of a .use(), so I'm not sure how much of an impact this change will be.

typescript-bot · 2020-10-10T01:04:45Z

@devanshj One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits or comments. If you disagree with the reviewer's comments, you can "dismiss" the review using GitHub's review UI. Thank you!

typescript-bot · 2020-10-10T15:34:14Z

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

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

typescript-bot · 2020-10-10T15:44:13Z

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

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

devanshj · 2020-10-10T15:51:05Z

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

+
+app.use(serveStatic('/infers-express-response-when-passed-to-express-use', {
+    setHeaders: function(res) {
+        // $ExpectType Response<any, number>


Not sure why but express.Response doesn't work, nor does express.Response<any, number> work. dtslint complains it needs to be Response<any, number>, so going with it.

typescript-bot · 2020-10-10T15:52:20Z

@rbuckton 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?

orta · 2020-10-16T15:42:32Z

Ping 🔔 @urossmolnik @LinusU - I'd like to get one more opinion on this PR from someone with experience using the library to make the trade-offs for the breaking change in the comments.

devanshj · 2020-10-16T16:26:17Z

Fwiw there isn't an option other than breaking it because the types were written incorrectly originally (not picking on anyone just objectively speaking).
Also important to note that the current types as they stand are actually incorrect in the sense they can cause runtime errors.

urossmolnik · 2020-10-16T17:01:59Z

Sorry haven't used this package in a while.
But I went through changes and looked at final file and it looks like it is aligned with serve-static implementation. As pointed out there should not be a named export - which was correctly fixed with this pull request.

orta · 2020-10-19T19:45:16Z

👍🏻

typescript-bot · 2020-10-19T20:10:09Z

I just published @types/serve-static@1.13.6 to npm.

sandersn · 2020-10-20T16:07:33Z

This breaks the tests for feathersjs__express. I'm investigating why; it's probably possible to work around fairly easily.

devanshj · 2020-10-20T16:24:27Z

This breaks the tests for feathersjs__express. I'm investigating why; it's probably possible to work around fairly easily.

Yeah the fix would be to annotate response parameter with express.Response, another (quick bad) fix could be explicitly passing express.Response as the type parameter.
Probably you'll figure it out, ping me if you want me to look into it.

devanshj added 2 commits October 7, 2020 23:27

add test showing incorrect assumption of response to be express response

f2eb7ee

make response generic to adhere to the implementation

dcb021a

typescript-bot added the Critical package label Oct 7, 2020

devanshj commented Oct 7, 2020

View reviewed changes

typescript-bot added the Perf: Same typescript-bot determined that this PR will not significantly impact compilation performance. label Oct 7, 2020

rbuckton requested changes Oct 10, 2020

View reviewed changes

typescript-bot added the Revision needed This PR needs code changes before it can be merged. label Oct 10, 2020

indicate node dependency via triple slash reference

7dfaef0

typescript-bot added The CI failed When GH Actions fails and removed Revision needed This PR needs code changes before it can be merged. labels Oct 10, 2020

devanshj force-pushed the serve-static-generic-response branch from cabf2b7 to 3c5457f Compare October 10, 2020 15:39

make tests stricter by expecting response type

fbb12c7

devanshj force-pushed the serve-static-generic-response branch from 3c5457f to fbb12c7 Compare October 10, 2020 15:47

devanshj commented Oct 10, 2020

View reviewed changes

typescript-bot removed the The CI failed When GH Actions fails label Oct 10, 2020

orta merged commit e445ccd into DefinitelyTyped:master Oct 19, 2020

This was referenced Oct 20, 2020

fix(express): update express.static for recent serve-static change #48964

Merged

fix(types): make request & response generic and remove express dependency chimurai/http-proxy-middleware#478

Closed

devanshj mentioned this pull request Nov 14, 2020

fix(serve-static): fix response constraint #49561

Merged

10 tasks

Conversation

devanshj commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

typescript-bot commented Oct 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

1 package in this PR

Code Reviews

Status

Uh oh!

typescript-bot commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

devanshj Oct 10, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Oct 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Oct 10, 2020

Uh oh!

typescript-bot commented Oct 10, 2020

Uh oh!

typescript-bot commented Oct 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

typescript-bot commented Oct 10, 2020

Uh oh!

orta commented Oct 16, 2020

Uh oh!

devanshj commented Oct 16, 2020

Uh oh!

urossmolnik commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

orta commented Oct 19, 2020

Uh oh!

typescript-bot commented Oct 19, 2020

Uh oh!

sandersn commented Oct 20, 2020

Uh oh!

devanshj commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

devanshj commented Oct 7, 2020 •

edited

Loading

typescript-bot commented Oct 7, 2020 •

edited

Loading

devanshj Oct 10, 2020 •

edited

Loading

urossmolnik commented Oct 16, 2020 •

edited

Loading