Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change Next err paramter to type any #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Change Next err paramter to type any #25

wants to merge 1 commit into from

Conversation

twrayden
Copy link

@twrayden twrayden commented Sep 16, 2020

Fixes: #24

I found that the type conflict comes from the err parameter type.

@types/express: (err?: any): void
compose-middleware: (err?: Error | null): void

This PR changes the Next err parameter type to any which matches @types/express.

Reproduction: https://github.com/thomasraydeniscool/compose-middleware-repro

@twrayden twrayden changed the title Fixes #24 Change Next err paramter to type any Sep 16, 2020
@blakeembrey
Copy link
Owner

I would have expected err | null to be assignable to any so this is odd. Can you add a test at all?

I'm a little hesitant to land this since it makes the library strictly worse for type safety.

@blakeembrey
Copy link
Owner

For example, for this to be typed correctly we'd need to change this to any:

err: Error,

@blakeembrey
Copy link
Owner

I took a look at your example and it's actually the bearer library that's incorrect here, not compose-middleware. The type signature says it may pass router to next(), but this library doesn't understand the magic strings that Express.js supports.

@blakeembrey
Copy link
Owner

blakeembrey commented Oct 3, 2020

It's caused by this line in Express.js:

(deferToNext: "router"): void;

This sounds like things work as expected, this library doesn't support deferToNext and therefore the types are working as expected. What's unexpected is probably that people are using the express.Handler definition without realizing this caveat. I'm not comfortable relaxing the typing here to support something that may actually break in production as a result (e.g. if someone actually calls next("router") and it doesn't work).

The library using express.Handler could be updated to define what it actually uses instead, or we can add next("router") support to this library instead.

@menocomp
Copy link

I have the same issue. Any plans to get this merged ?

@blakeembrey
Copy link
Owner

If you’d like to contribute a test case using the types I can look at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Type 'Next<void>' is not assignable to type 'NextFunction'
3 participants