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

Expose run options in UseCls decorator #85

Closed
TTiole opened this issue Aug 29, 2023 · 5 comments · Fixed by #105
Closed

Expose run options in UseCls decorator #85

TTiole opened this issue Aug 29, 2023 · 5 comments · Fixed by #105
Labels
enhancement New feature or request
Milestone

Comments

@TTiole
Copy link

TTiole commented Aug 29, 2023

The UseCls decorator is very convenient; however, I need to use the IfNested option and unfortunately the UseCls decorator does not expose it.
It would be great if you could expose the IfNested option (and any other options you may add to .run in the future) on the UseCls decorator options.

@Papooch
Copy link
Owner

Papooch commented Aug 29, 2023

The UseCls decorator was meant only to initialize the context once, the same as the middleware or interceptor does but in cases where those are not available - that is, it should mark the entry point into the CLS context. I would not like to mix initializing the context with creating nested contexts.

However, I might consider adding new separate decorator that would mimic cls.run with all its options.

Can you share your actual use-case, so I can take some inspiration from it?

@Papooch Papooch added enhancement New feature or request needs info Waiting for more information from the OP labels Aug 29, 2023
@Papooch
Copy link
Owner

Papooch commented Dec 19, 2023

Closing this as not planned.

If anyone needs such feature, it can be implemented in userland with:

export function RunWithCls(options: ClsContextOptions) {
  return return (
        target: any,
        propertyKey: string | symbol,
        descriptor: TypedPropertyDescriptor<(...args: any[]) => any>,
    ) => {
        const cls = ClsServiceManager.getClsService();
        const original = descriptor.value;
        descriptor.value = function (...args) {
            return cls.run(options, () => original.apply(this, args));
        };
    };
}

@Papooch Papooch closed this as completed Dec 19, 2023
@TTiole
Copy link
Author

TTiole commented Dec 19, 2023

Ah, sorry I completely missed your first message. The use case is as follows:

We have a "main" HTTP server which consumes requests and a worker which consumes jobs from queues & crons.

There is some code we want to share between these that relies on Cls being initialized. Thus, the solution is to effectively "upsert" the single cls context. If it doesn't exist (which it usually doesn't in the worker) then create it with some defaults. Otherwise, leave it untouched.

IfNested allows me to do this.

In a sense, it's still initializing once, it's simply not initializing if it's already initialized.

@Papooch Papooch closed this as not planned Won't fix, can't repro, duplicate, stale Dec 20, 2023
@Papooch
Copy link
Owner

Papooch commented Dec 20, 2023

I see, I would still argue that, in this scenario, it's better to initialize the context explicitly on the calling side as soon as the request (or job) is received, so there's a single entry point to the context per caller, rather than two possible entry points depending on the caller - and to have it fail when no context was initialized in the caller.

This would prevent bugs such as failure to initialize the context in the "main" app on some route causing silently initializing the wrong context in the called function. Just recently, a bug in NestJS (#92) caused the ClsMidleware to not be bound properly when a global prefix was set, so you would've never caught this if the context was implicitly initialized with default values every time.

However, creating a decorator that supports your scenario is pretty straightforward to implement, so if you're still not convinced, feel free to use the code I shared in the previous comment.

@Papooch Papooch added wontfix This will not be worked on and removed enhancement New feature or request needs info Waiting for more information from the OP labels Dec 20, 2023
@Papooch Papooch reopened this Jan 22, 2024
@Papooch Papooch added enhancement New feature or request and removed wontfix This will not be worked on labels Jan 22, 2024
@Papooch
Copy link
Owner

Papooch commented Jan 22, 2024

I finally realized that it is not up to me how you want to initialize the context and that pragmatism shouldn't stand second to coretness.

Using the @UseCls decorator should be at least equivalent feature-wise to cls.run(), which means the options should be exposed, so this feature will be available in the following v4 as

@UseCls({ runOptions: { ifNested: 'reuse' } })

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