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

Fix type information to allow express middlewares #146

Closed
tobiasdiez opened this issue Jul 20, 2022 · 6 comments
Closed

Fix type information to allow express middlewares #146

tobiasdiez opened this issue Jul 20, 2022 · 6 comments

Comments

@tobiasdiez
Copy link
Contributor

If you use an express-middleware (such as session or passport), one gets the following typescript error:

Type 'RequestHandler<ParamsDictionary, any, any, ParsedQs, Record<string, any>>' is not comparable to type 'EventHandler<any>'.

Example-code:

import session from 'express-session'
import passport from 'passport'
import { createApp } from 'h3'

const app = createApp()
app.use(session())
app.use(passport.initialize())
@NozomuIkuta
Copy link
Member

@tobiasdiez

@pi0 explains it in #73 why they are changing middleware signature.

In my understanding, h3 is aiming to be environment-agnostic server framework for future, even with trade-offs like abandoning the existing ecosystem.

One possible compromise might be to provide a glue function that convert event from req/res and vice versa, so that we can wrap legacy middlewares, as well as letting the community recreate h3-dedicated middlewares.

// This is a virtual code 
// `eventify` passes `req/res` and `next` function to the wrapped middleware
app.use(eventify(session()))

@tobiasdiez
Copy link
Contributor Author

Sorry, I should have been more precise. There is already such a wrapper in place (und the hood) and the above code does work. It's only that TypeScript is throwing an error, so I guess something is off here

export type Middleware = (req: IncomingMessage, res: ServerResponse, next: (err?: Error) => any) => any

@NozomuIkuta
Copy link
Member

NozomuIkuta commented Aug 5, 2022

Sorry, I misunderstood you.

By the way, I couldn't instantly reproduce the error that you are taking about.
Would you provide a link to reproduction, so that I can investigate it more deeply?

@tobiasdiez
Copy link
Contributor Author

@NozomuIkuta please have look at this ts Playground. The app.use calls are showing ts errors.

@tobiasdiez tobiasdiez changed the title Support express middleware Fix type information to allow express middlewares Aug 31, 2022
@NozomuIkuta
Copy link
Member

@tobiasdiez

For your information, I found that @pi0 had created a branch where he is removing compatibility with legacy Express-style middleware whose signature is (req, res, next?) => {}.


By the way, it's written in README that:

For maximum compatibility with connect/express middleware (req, res, next? signature), h3 converts classic middleware into a promisified version ready to use with stack runner:

Once the branch is merged, the "maximum" compatibility get to be "zero" (note: this change has been planned in #73).

So, at this moment, all options we have would be (1) to make up h3-native auth solution by ourselves rather than making h3 complex by adding Express middleware support, or (2) to give a support to authors so that they can do it for us (e.g. sponsoring via GitHub). That's just because h3 is an OSS.

@pi0
Copy link
Member

pi0 commented Oct 18, 2022

Middleware made for express, depend on specific auegmentations coming from express app itself (.json(), .cookie(), etc)

In order to use express middleware with h3, you need to first create an express app:

const app = h3.createApp()

const expressApp = express()
expressApp.use(expressMiddleware()

app.use(h3.fromNodeMiddleware(expressApp))

@pi0 pi0 closed this as completed Oct 18, 2022
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

No branches or pull requests

3 participants