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

♻️ [RUMF-1505] make sure we don't use Zone.js addEventListener #2129

Conversation

BenoitZugmeyer
Copy link
Member

@BenoitZugmeyer BenoitZugmeyer commented Mar 31, 2023

Motivation

Follow-up of related PRs:

We observed that using some addEventListener caused some critical issues when this function is patched by Zone.js. To work around that, the ESLint rule disallow-zone-js-patched-values is used to make sure we don't use addEventListener directly but instead use the "safe" addEventListener helper from @datadog/browser-core.

Changes

  • Forbid direct addEventListener usages
  • Use the addEventListener utility functions everywhere we used
    addEventListener native methods.

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

Copy link
Member Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@codecov-commenter
Copy link

Codecov Report

Merging #2129 (f2806b1) into benoit/improve-add-event-listener-typings (34472d0) will increase coverage by 0.08%.
The diff coverage is 93.54%.

@@                              Coverage Diff                              @@
##           benoit/improve-add-event-listener-typings    #2129      +/-   ##
=============================================================================
+ Coverage                                      93.56%   93.65%   +0.08%     
=============================================================================
  Files                                            179      179              
  Lines                                           5984     5987       +3     
  Branches                                        1342     1342              
=============================================================================
+ Hits                                            5599     5607       +8     
+ Misses                                           385      380       -5     
Impacted Files Coverage Δ
packages/core/src/browser/addEventListener.ts 100.00% <ø> (ø)
packages/rum/test/mockWorker.ts 98.57% <80.00%> (-1.43%) ⬇️
...ckages/rum/src/domain/segmentCollection/segment.ts 97.29% <92.30%> (ø)
packages/core/src/browser/xhrObservable.ts 98.03% <100.00%> (ø)
packages/core/src/transport/httpRequest.ts 97.91% <100.00%> (-0.05%) ⬇️
...ages/rum-core/src/browser/performanceCollection.ts 52.04% <100.00%> (ø)
...src/domain/segmentCollection/startDeflateWorker.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Base automatically changed from benoit/improve-add-event-listener-typings to main April 4, 2023 10:16
@BenoitZugmeyer BenoitZugmeyer force-pushed the benoit/disallow-zone-js-patched-values--add-event-listener branch from f2806b1 to c0cd6b5 Compare April 4, 2023 10:18
@BenoitZugmeyer BenoitZugmeyer marked this pull request as ready for review April 4, 2023 11:07
@BenoitZugmeyer BenoitZugmeyer requested review from a team as code owners April 4, 2023 11:07
@BenoitZugmeyer BenoitZugmeyer merged commit 92b0e6a into main Apr 5, 2023
2 checks passed
@BenoitZugmeyer BenoitZugmeyer deleted the benoit/disallow-zone-js-patched-values--add-event-listener branch April 5, 2023 08:22
bcaudan added a commit that referenced this pull request Apr 5, 2023
 pm_trace_id: 14645840
 feature_branch_pipeline_id: 14645840
 source: to-staging

* commit '2e524118ea73a9b975bc25f095826e914df2a10d':
  ♻️ [RUMF-1505] make sure we don't use Zone.js addEventListener (#2129)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants