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

Add dependancy injection for handlers #230

Closed
Isaac-Leonard opened this issue Dec 10, 2021 · 6 comments · Fixed by #234
Closed

Add dependancy injection for handlers #230

Isaac-Leonard opened this issue Dec 10, 2021 · 6 comments · Fixed by #234
Labels
enhancement New feature or request

Comments

@Isaac-Leonard
Copy link

Currently the package provides a logger in a sort of DI way which is extremely useful.
I believe it should be possible to generalise this so users could add their own dependancy objects to pass to handlers when building endpoints.
I've got some code that can sort of do this already however it is not perfect yet and would be a breaking change when it shouldn't be so not ready for a pull request yet.
An example usage would be

new EndPointFactory(...)
  .addDependancy({db:SomeDataBaseObject})
  .build({
    input:...
    output:...
    handler: async ({input, db, logger})=>{
      db.query();
      return ...;
    }),
  });

Sorry for any mistakes, its 04:00 am here.
What do you think of this?
I'd also like to expand it to add dependancies that get created with a function on each request too for stuff like specific database connections instead of a single database object.

@RobinTail
Copy link
Owner

RobinTail commented Dec 11, 2021

@Isaac-Leonard , for me this method looks redundant, since you're connecting this db dependency in the same place where your handler is implemented, so you can use it directly within handler without passing it through the factory.

In case this entity somehow depends on incoming data or available for limited users, the existing method .addMiddleware() is designed for passing the desired entity to the options parameter of the handler.

Perhaps I'm missing something. Please let me know if these two solutions are not suitable for your case (with some more details). Thank you!

@Isaac-Leonard
Copy link
Author

Actually yeah you're right, I really should read the full documentation first before trying to add features.bThe only differences I would maybe make is to add the properties on options directly to the argument passed to handler instead of as a options property but really that doesn't make much of a difference and not requiring the createMiddleware boilerplate..

@Isaac-Leonard
Copy link
Author

Actually no
Looking at it more there is currently no typing on the options object.
Thats what My solution would add.
I've pushed my changes on my fork if you would like to look although they are a bit messy, its got tests demonstrating its typesafeness.
Currently it requires a breaking change as typescript can't infer types without it.
It currently allows for anything to be added to a dependancies object that gets passed to the final handler but only static data for now, making it dynamic such as creating individual db connections or authenticating users with a type safe user object being added to the result however should be doable.
The current options object is just typed as any.

@RobinTail
Copy link
Owner

The current options object is just typed as any.

I'd recommend to check the type of your middleware's output and how you connect this middleware.

Usually it works this way (in a project, that utilizes express-zod-api):

image

@RobinTail
Copy link
Owner

and not requiring the createMiddleware boilerplate..

I agree, that in some cases, especially for empty inputs, there may be some sort of shorthand for providing Endpoints with options easier. Need to think on it a little bit...

@RobinTail
Copy link
Owner

@Isaac-Leonard , please check out PR #234.
Does it fulfill your needs?

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

Successfully merging a pull request may close this issue.

2 participants