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

Prevent double wrapping event emitter listeners #3133

Merged
merged 7 commits into from
Aug 3, 2022

Conversation

dyladan
Copy link
Member

@dyladan dyladan commented Aug 1, 2022

Fixes #2971

This is an alternative solution to #3132

@dyladan dyladan requested a review from a team as a code owner August 1, 2022 13:44
@dyladan dyladan changed the title Add failing tests for #2971 Prevent double wrapping event emitter Aug 1, 2022
@dyladan dyladan changed the title Prevent double wrapping event emitter Prevent double wrapping event emitter listeners Aug 1, 2022
@codecov
Copy link

codecov bot commented Aug 1, 2022

Codecov Report

Merging #3133 (dd40ed0) into main (dc67f4e) will decrease coverage by 1.95%.
The diff coverage is n/a.

❗ Current head dd40ed0 differs from pull request most recent head aee61dd. Consider uploading reports for the commit aee61dd to get more accurate results

@@            Coverage Diff             @@
##             main    #3133      +/-   ##
==========================================
- Coverage   93.10%   91.15%   -1.96%     
==========================================
  Files         196       68     -128     
  Lines        6400     1855    -4545     
  Branches     1349      395     -954     
==========================================
- Hits         5959     1691    -4268     
+ Misses        441      164     -277     
Impacted Files Coverage Δ
packages/opentelemetry-resources/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...ckages/opentelemetry-sdk-trace-web/karma.worker.js 0.00% <0.00%> (-100.00%) ⬇️
...s/opentelemetry-core/src/platform/node/sdk-info.ts 0.00% <0.00%> (-100.00%) ⬇️
...opentelemetry-core/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...pentelemetry-core/src/platform/node/performance.ts 0.00% <0.00%> (-100.00%) ⬇️
...emetry-api-metrics/src/platform/node/globalThis.ts 0.00% <0.00%> (-100.00%) ⬇️
...lemetry-resources/src/detectors/ProcessDetector.ts 31.81% <0.00%> (-68.19%) ⬇️
...perimental/packages/otlp-exporter-base/src/util.ts 79.41% <0.00%> (-17.65%) ⬇️
...ckages/opentelemetry-exporter-zipkin/src/zipkin.ts 84.48% <0.00%> (-15.52%) ⬇️
...lemetry-resources/src/detectors/BrowserDetector.ts 93.33% <0.00%> (-6.67%) ⬇️
... and 128 more

@dyladan dyladan added bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc labels Aug 1, 2022
@dyladan
Copy link
Member Author

dyladan commented Aug 1, 2022

@vmarchaud I know you already approved a different one but I prefer this solution. It depends on the sync nature of node but doesn't depend on any implementation details of on/once or the listener

@vmarchaud
Copy link
Member

I know you already approved a different one but I prefer this solution. It depends on the sync nature of node but doesn't depend on any implementation details of on/once or the listener

This one is also fine to me (since it don't modify the bind fn).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working priority:p1 Bugs which cause problems in end-user applications such as crashes, data inconsistencies, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AbstractAsyncHooksContextManager breaks .removeListener for handlers which was added with .once
4 participants