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

Pass action (request and response) when resolving middleware from container #1034

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

alenkralj
Copy link

Description

With in mind that resolving instances is done via:

interface IocAdapter {
  get<T>(someClass: ClassConstructor<T>, action?: Action): T;
}

When resolving controllers and interceptors action parameter is passed, while for middlewares it is not. Perhaps it should be consistent.

My goal is to have a HTTP request scoped containers. Meaning, if I want to have an injectable class e.g. RequestContext which contains information about the request that other services can consume I must have only one instance per HTTP request for such class. I could achieve this by opening child container for each HTTP request and bind it as a constant. The problem is where to keep child container. My current thinking is it should be part of the request, like:

export class InversifyAdapter implements IocAdapter {
  constructor(private readonly container: Container) {}

  get<T>(someClass: ClassConstructor<T>, action?: Action): T {
    if (!action) {
      // global resolution
      return this.container.get<T>(someClass)
    }

    if (!action?.request?.iocContainer) {
      action.request.iocContainer = this.container.createChild()
      // ... bind stuff
    } 

    // request-scoped resolution
    return action.request.iocContainer.get<T>(someClass)
  }
}

Which works fine for e.g. controllers, but not for middlewares which have injected dependencies through a constructor. As a workaround I can in middleware use method resolve dependencies via request.iocContainer.

Checklist

  • the pull request title describes what this PR does (not a vague title like Update index.md)
  • the pull request targets the default branch of the repository (develop)
  • the code follows the established code style of the repository
    • npm run prettier:check passes
    • npm run lint:check passes
  • tests are added for the changes I made (if any source code was modified)
  • documentation added or updated
  • I have run the project locally and verified that there are no errors

Copy link

@tomislavherman tomislavherman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems very simple fix with a big gain. Hope it will be merged soon since I'm using ugly hack in my project to overcome this.

@@ -58,17 +58,19 @@ export class ExpressDriver extends BaseDriver {
let middlewareWrapper;

// if its an error handler then register it with proper signature in express
if ((middleware.instance as ExpressErrorMiddlewareInterface).error) {
if (middleware.target.prototype && middleware.target.prototype.error) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can simplify expression as if (middleware.target?.prototype?.error). Same for others here.

@gayanhewa
Copy link

@alenkralj @tomislavherman I haven't had a use case personally so far to use a request-scoped container. What is a good example use case 🤔

@attilaorosz
Copy link
Member

I'll be honest, this looks like a memory leak :).
If you want to have a scoped context I suggest to use AsyncLocalStorage instead (https://nodejs.org/api/async_context.html#class-asynclocalstorage), which is a native module.

I believe that passing the request and response when resolving is probably a mistake that was not removed previously but If we find a use case then we can implement this.

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

Successfully merging this pull request may close these issues.

4 participants