-
Notifications
You must be signed in to change notification settings - Fork 823
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
feat: use a symbol to store patched listeners #1356
feat: use a symbol to store patched listeners #1356
Conversation
Use a symbol instead a string property to avoid clashes in case several independent instances of AsyncHooksContextManager are present.
Codecov Report
@@ Coverage Diff @@
## master #1356 +/- ##
==========================================
+ Coverage 94.11% 94.13% +0.01%
==========================================
Files 145 145
Lines 4318 4314 -4
Branches 880 877 -3
==========================================
- Hits 4064 4061 -3
+ Misses 254 253 -1
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
change seems fine, and preventing these clashes is always a good thing, but what is the use-case for multiple context managers in a single project?
I don't think that this is a real usecase someone plans but it can just happen. Consider a setup like dev team is using OTel and Operations adds an APM. The APM vendor may have a copy of OTel included. There is also a possibility that someone just uses/forks the context manager from OTel to create something completely new. In short, bigger nodejs applications sometimes tend to accumulate a lot stuff together... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Which problem is this PR solving?
There might be several instances of
AsyncHooksContextManager
present which would result in clashes.Short description of the changes
Use a symbol instead a string property to avoid clashes in case several independent instances of
AsyncHooksContextManager
are present.