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-1580] Decouple storage mechanism #2259

Merged
merged 13 commits into from
Jun 13, 2023

Conversation

yannickadam
Copy link
Contributor

@yannickadam yannickadam commented May 19, 2023

Motivation

Before implementing alternate storage mechanism, decouple sessionStore from sessionCookieStore.

Changes

  • Created a SessionStore interface to which every storage must comply
  • Refactored sessionCookieStore, now implements SessionStore
  • Moved the storage lock mechanism from SessionCookieStore to SessionStore for reusability with other storages

Testing

  • Local
  • Staging
  • Unit
  • End to end

I have gone over the contributing documentation.

@yannickadam
Copy link
Contributor Author

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@yannickadam yannickadam changed the title [RUMF-1580] Decouple storage mechanism ♻️ [RUMF-1580] Decouple storage mechanism May 19, 2023
@yannickadam yannickadam force-pushed the yannick.adam/RUMF-1580_Decouple_storage_mechanism branch from 4fca135 to e4af055 Compare May 22, 2023 09:23
@codecov-commenter
Copy link

codecov-commenter commented May 22, 2023

Codecov Report

Merging #2259 (72f4303) into main (179d8df) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #2259      +/-   ##
==========================================
- Coverage   93.81%   93.79%   -0.03%     
==========================================
  Files         201      203       +2     
  Lines        6083     6090       +7     
  Branches     1347     1348       +1     
==========================================
+ Hits         5707     5712       +5     
- Misses        376      378       +2     
Impacted Files Coverage Δ
...ges/core/src/domain/session/oldCookiesMigration.ts 100.00% <100.00%> (ø)
...ages/core/src/domain/session/sessionCookieStore.ts 100.00% <100.00%> (ø)
packages/core/src/domain/session/sessionManager.ts 100.00% <100.00%> (ø)
packages/core/src/domain/session/sessionStore.ts 100.00% <100.00%> (ø)
...ges/core/src/domain/session/sessionStoreManager.ts 100.00% <100.00%> (ø)
.../core/src/domain/session/sessionStoreOperations.ts 100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

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

@yannickadam yannickadam force-pushed the yannick.adam/RUMF-1580_Decouple_storage_mechanism branch from e4af055 to 20b0d29 Compare May 30, 2023 12:52
@yannickadam yannickadam marked this pull request as ready for review May 30, 2023 13:53
@yannickadam yannickadam requested a review from a team as a code owner May 30, 2023 13:53
packages/core/src/domain/session/sessionManager.spec.ts Outdated Show resolved Hide resolved
packages/core/src/domain/session/sessionStoreOperations.ts Outdated Show resolved Hide resolved
bufferedOperations.push(operations)
return
}
if (lockEnabled && numberOfRetries >= LOCK_MAX_TRIES) {
Copy link
Contributor

Choose a reason for hiding this comment

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

💬 suggestion: ‏is it needed?

Suggested change
if (lockEnabled && numberOfRetries >= LOCK_MAX_TRIES) {
if (numberOfRetries >= LOCK_MAX_TRIES) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somehow yes - If lockEnabled is removed - the code works only because LOCK_MAX_TRIES > 0. I'd prefer not to check the lock retry mechanism when locks are not enabled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Setting LOCK_MAX_TRIES to 0 seems unlikely but I get the intent.
Could we make a test to enforce that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@yannickadam yannickadam merged commit cd7dcc8 into main Jun 13, 2023
@yannickadam yannickadam deleted the yannick.adam/RUMF-1580_Decouple_storage_mechanism branch June 13, 2023 09:00
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.

4 participants