awesometic / nestjs-request-id

A NestJS module for tracking the request flows by adding unique request ID
MIT License
5 stars 1 forks source link

Request scoped #14

Closed CaptainQuirk closed 9 months ago

CaptainQuirk commented 9 months ago

Hi,

I stumbled upon your module while trying to solve a problem related to provider scope.

Wouldn't your request scoped RequestIdService make every provider that depend on it request scoped ?

awesometic commented 9 months ago

Hello,

No, as far as I know a module that has the Request scope doesn't affect the other modules' scope. That scope just means when/how the injectable provider initialized and destroyed.

https://docs.nestjs.com/fundamentals/injection-scopes

But please tell me if I misunderstood. Thanks :)

CaptainQuirk commented 9 months ago

In the Scope Hierarchy section, one can read the following :

The REQUEST scope bubbles up the injection chain. A controller that depends on a request-scoped provider will, itself, be request-scoped.

Imagine the following dependency graph: CatsController <- CatsService <- CatsRepository. If CatsService is request-scoped (and the others are default singletons), the CatsController will become request-scoped as it is dependent on the injected service. The CatsRepository, which is not dependent, would remain singleton-scoped.

awesometic commented 9 months ago

Ah, right, if a controller has a Request scoped provider, then the other providers are changed to Request scoped too.

We can do a simple test by creating a provider that has Default scope and this Request ID module. Sending multiple requests, and make one of them change a property of the Default scoped provider, and read that value from all the requests. In the Default provider, it acts like a singleton object in the project, so it should share the changed property, which leads to all the requests showing the same property value. I guess. But if this Request scoped provider makes the other module also Request scoped, the property won't be changed since all the providers are initialized with each request.

If this is true, maybe using an event bus can avoid this issue. But we need also some tests on it anyway to confirm. 🤔

CaptainQuirk commented 9 months ago

If you set a NEST_DEBUG environment variable with true as value in your .env file, you'll see in the console the scope in which you providers are resolved. The only solution I found to handle this, is described here and relies on declaring a middleware wrapping the next() function with the built-in nodejs AsyncLocalStorage

awesometic commented 9 months ago

Got it, so it seems like my source code has to be patched to resolve the issue 😮 At least, it seems like using Request scope should be removed if I understand well.

Thank you for raising the issue. I learned one more thing thanks to you 😄 If you have spare time, please make a PR to this repository, I'd like to discuss that.