feat: proposal for server agnostic handlers sample #2150
Conversation
|
Add missing await
| expressApp.use('/oid4vci', issuer.agent.modules.openId4VcIssuer.config.router) | ||
| expressApp.use( | ||
| '/oid4vci', | ||
| (issuer.agent.dependencyManager.registeredModules['openId4VcIssuer'] as OpenId4VcIssuerModule).contextRouter |
There was a problem hiding this comment.
This should be easier, as users of the frameworks have to do this registration step as well. It should not rely on dependency injection or type casting. this is also why the router was available on the module config (where I think it should stay)
| export interface OpenId4VcRequest<RC extends Record<string, unknown> = Record<string, never>> extends Request { | ||
| requestContext?: RC & OpenId4VcRequestContext | ||
| requestContext?: OpenId4VcRequestContext<RC> | ||
| } | ||
|
|
||
| export interface OpenId4VcRequestContext { | ||
| export type OpenId4VcRequestContext<RC extends Record<string, unknown> = Record<string, never>> = RC & { | ||
| agentContext: AgentContext |
There was a problem hiding this comment.
why is this change needed?
There was a problem hiding this comment.
thanks for the PR, it helps to understand the approach. My thinking was a bit different, and have better separation of express from the main implementaiton. As now it's sort of implemented around express, but you can 'hack' your way around it.
A problem with this model is that you now have to depend on internal apis (registerXXEndpoint methods).
I also don't like that we have to move the router to the module (but i think this can be solved by not making the config or module dependant on express at all anymore).
I think we just need an interface to register an endpoint? You can provide that to Credo. What we have to support:
- variables in the path
- method
- maybe more??
we will have to rewrite some middleware (around issuerId and issuer injection), but if we go generic, I think it's important to do it properly.
Again, thanks a lot for this PR, it's really appreciated :)
| /* | ||
| * Copyright 2025 Velocity Team | ||
| * | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * | ||
| */ |
There was a problem hiding this comment.
I know this is not important at this stage of the PR, but just as an FYI:
I would rather not add these headers to all files. IF you want copyright credited, it can be added to the main license file (see example from Ontario: https://github.com/openwallet-foundation/credo-ts/blob/main/LICENSE#L190)
|
And as I said in the issue, I think it might make sense to rely on native web Request/Response format. An issue with the current implementation I see is that your custom http implementation now needs to know about errors like If we want the endpoint implementations to be part of the public api, it should take care of this. Otherwise everytime we add a new error type, it will become a breaking change. For example the Next.js api for adding routes depends on the |
@TimoGlastra
I wanted to see it in code and get your initial thoughts on the following to complete #2147. there is a little bit of cleanup still to do but i think you get the gist.
The main issues that I want to draw your attention to are:
handlernot being dependent onexpress, onlynode:httpXXXModuleConfignot being dependent onexpress, only theXXXModuleXXXModulerouter when required