-
-
Notifications
You must be signed in to change notification settings - Fork 462
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
feat: improve h3 middleware #1155
Conversation
@@ -17,6 +17,7 @@ import type { | |||
Response as ExpressResponse | |||
} from "express"; | |||
import type { FastifyReply, FastifyRequest } from "fastify"; | |||
import { H3Event, getCookie, getRequestURL, setCookie } from 'h3' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't allow runtime dependencies. I'd rather not have users have issue with unrelated dependencies.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be okay to add it as peer-dependency?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how that improves it? Should've said "We discourage adding runtime dependencies for middlewares"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Peer dependencies have to be installed manually, so users will not have issues with unrelated deps.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see how peer deps is related here? Either way, I just don't want to add dependencies just to get middlewares working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern is adding additional dependencies that we don't really need (e.g. the setCookie()
method is pretty much the same as the one used in Node middleware), and making library/framework specific dependencies optional for users not using that framework. I don't think setting it as a peer dependency is helpful here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should I then implement it as a new h3 middlware package?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like I commented, if the current middleware is not working, please open a new issue with a reproduction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's sadly not easily reproducible as I only experienced the issues when deploying to azure. Together with the fix unjs/nitro#1753 and this PR, it works well.
Either way, its only a matter of time until h3 will change their get/setCookies implementation since they want to remove the node dependency and instead use the events api (unjs/h3#73). Actually, the missing implementation for cookies is the only reason why one cannot use this PR and deploy it on deno.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even without a reproduction, can you open a new bug report with some code examples?
Can you provide a bug report first (and maybe a reproduction)? |
- Once redis/ioredis#1822 is fixed, remove patch to ioredis - Once Azure/azure-functions-host#162 is fixed, remove patch to nitro that wraps console log. - Once unjs/nitro#1753 is merged and released, remove corresponding patch to nitro - Once lucia-auth/lucia#1153 is fixed, rename models/fields in prisma - Once lucia-auth/lucia#1155 is merged and released, remove custom h3 lucia middleware - Once lucia-auth/lucia#1074 is fixed, remove lucia types shims - Enable CSRF protection in lucia (but then login doesn't work anymore...)
I had some problems with the provided h3 middleware (e.g. cookies were not send correctly), so I've reimplemented the middleware without using node as an in-between layer.