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-1517] reorganise some components #2124

Merged
merged 7 commits into from
Apr 3, 2023
Merged

Conversation

bcaudan
Copy link
Contributor

@bcaudan bcaudan commented Mar 30, 2023

Motivation

Housekeeping + preventing potential cyclic dependencies

Changes

  • move error to domain
  • move eventRateLimiter to domain
  • move runOnReadyState to browser
  • move experimentalFeatures to tools
  • move timer to tools

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

Copy link
Contributor Author

bcaudan commented Mar 30, 2023

if it was domain logic, we should not use it on tools code
it had dependency on domain
since it manipulates RawError, it should be on domain
while it wraps a browser API, it is mainly used to control the execution flow
dependency on adEventListener and highly browser specific
@bcaudan bcaudan marked this pull request as ready for review March 30, 2023 13:27
@bcaudan bcaudan requested a review from a team as a code owner March 30, 2023 13:27
@bcaudan bcaudan changed the title ♻️[RUMF-1517] add tools index ♻️[RUMF-1517] reorganise some components Mar 31, 2023
@codecov-commenter
Copy link

Codecov Report

Merging #2124 (44baad2) into main (c466c8f) will increase coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #2124      +/-   ##
==========================================
+ Coverage   93.60%   93.64%   +0.03%     
==========================================
  Files         178      178              
  Lines        5976     5976              
  Branches     1341     1341              
==========================================
+ Hits         5594     5596       +2     
+ Misses        382      380       -2     
Impacted Files Coverage Δ
packages/core/src/browser/pageExitObservable.ts 100.00% <ø> (ø)
packages/core/src/browser/runOnReadyState.ts 100.00% <ø> (ø)
...ges/core/src/domain/configuration/configuration.ts 92.50% <ø> (ø)
...s/core/src/domain/configuration/endpointBuilder.ts 100.00% <ø> (ø)
...kages/core/src/domain/console/consoleObservable.ts 100.00% <ø> (ø)
packages/core/src/domain/error/error.ts 92.59% <ø> (ø)
...ackages/core/src/domain/error/trackRuntimeError.ts 100.00% <ø> (ø)
.../domain/eventRateLimiter/createEventRateLimiter.ts 100.00% <ø> (ø)
...ackages/core/src/domain/report/reportObservable.ts 95.00% <ø> (ø)
...ages/core/src/domain/session/sessionCookieStore.ts 100.00% <ø> (ø)
... and 14 more

... and 1 file with indirect coverage changes

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❓ question: ‏I'm wondering what made you move timer.ts in tools? For me, this module is similar to addEventListener.ts (provide safer or easier to use alternative to browser functions). Is it because setTimeout is more "JavaScript-y" and addEventListener is more DOM-oriented (thus "browser")?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is what I tried to express with:

while it wraps a browser API, it is mainly used to control the execution flow

Components needing to react to window or document events would most likely be under domain or browser directories.
For timer APIs, any component needing to perform asynchronous operation could need them and we have usage from various areas of the code base.

The main driver is that it would avoid to have some tools components depending on code outside of tools.

@bcaudan bcaudan mentioned this pull request Mar 31, 2023
4 tasks
@bcaudan bcaudan merged commit ff79c34 into main Apr 3, 2023
2 checks passed
@bcaudan bcaudan deleted the bcaudan/tools-index branch April 3, 2023 07:29
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