-
Notifications
You must be signed in to change notification settings - Fork 52
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
First steps of adding async hooks #135
Conversation
I personally think that the purpose of this module is to function as a sort of fallback in case AsyncHooks isn't available, but I think it's better to leave the async hooks implementation up the consumer of this module instead of baking it into the module it self. But there might be a use-case that I hadn't thought about? |
Hey, thanks for the answer, my current use case is: To wrap up, all I need is the async-listener API based on Async Hooks in order to exploit that in the CLS without changing my code. WDYT ? |
It's not a big maintainability overhead to add async hook support to this module, so as such I'm not against it. But I'd like to hear some thoughts from the other maintainers of this module. In any case, if we go ahead with this, I think existing |
I think for CLS the better option is to use |
CLS is universally just trying to link async barriers to form that context though, so whether it is async_hooks, AsyncListener, or the monkey patches provided in the async-listener abstraction, they all achieve the same goal. I think it makes sense to have one universal module that provides that abstraction over all the underlying tech options to allow the functionality to exist in a performant way across a wider range of Node.js versions. |
If there's a desire for a low-level interface/abstraction that can be used by older versions of node, wouldn't it make more sense to create an |
I don't think a complete polyfill of async_hooks is possible. It'd have to be slightly abstract to reduce it to only the parts of async_hooks that are actually reproduce-able in userland, which this module already does. |
@Qard explained why I wanted this PR better than I could. ATM, I have reimplemented a clone of the CLS in my agent but it is basically a copy of the sources with a few changes. Maybe a change in the CLS rather than here would make more sense? Giving CLS-hooked, I'm not keen about having two different CLS at once from external sources. |
Hey,
I started a tentative of adding async hooks here. I still have a few issues to fix, but I wanted to check if:
Wdyt?