Initial attempt to decouple persistence #832

alexs-mparticle · 2024-01-19T17:29:21Z

Instructions

PR target branch should be against development
PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

{provide a thorough description of the changes}

Testing Plan

Was this tested locally? If not, explain why.
{explain how this has been tested, and what, if any, additional testing should be done}

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

Closes https://go.mparticle.com/work/REPLACEME

rmi22186

This looks good so far. Here are my comments for Monday morning. I didn't look at the tests since they may change given my feedback.

rmi22186 · 2024-01-19T21:00:20Z

src/sessionManager.ts

+                persistence?.gs &&
+                persistence?.gs.sid &&


Suggested change

persistence?.gs &&

persistence?.gs.sid &&

persistence?.gs?.sid &&

rmi22186 · 2024-01-19T21:00:36Z

src/sessionManager.ts

            mpInstance._Store.SDKConfig.sessionTimeout * 60000;

        mpInstance._Store.globalTimer = window.setTimeout(function() {
+            // TODO: kill this global timer when ending session


rmi22186 · 2024-01-19T21:04:16Z

src/sessionManager.ts

+            persistence?.gs &&
+            persistence?.gs.sid &&


Suggested change

persistence?.gs &&

persistence?.gs.sid &&

persistence?.gs?.sid &&

src/mp-instance.js

rmi22186 · 2024-01-19T22:38:43Z

src/sessionManager.ts

+            persistence?.gs &&
+            persistence?.gs.les &&


Suggested change

persistence?.gs &&

persistence?.gs.les &&

persistence?.gs?.les &&

rmi22186 · 2024-01-19T22:40:22Z

src/sessionManager.ts

+        // debugger;

-        if (cookies.gs && !cookies.gs.sid) {
+        // QUESTION: What constitutes ending a session if we don't have persistence?


Addressed in earlier comments, but if we don' have persistence, then the expectation is that everything is saved to the store, so the logic would be if persistence is disabled, check the store instead

rmi22186 · 2024-01-19T22:43:08Z

src/sessionManager.ts

                    messageType: Types.MessageType.SessionEnd,
                });

-                mpInstance._Store.sessionStartDate = null;


I think line 182 was removed erroneously.

I intentionally moved it to a new method Store.nullifySession() since it made sense to me to keep this grouped with the other nullify session logic. Is there a reason to not nullify the session start date when we nullify a session?

rmi22186 · 2024-01-19T22:44:05Z

src/sessionManager.ts

+                mpInstance._Store.sessionId = persistence.gs.sid;
+            }
+
+            // TODO: Make this a store method


Can you clarify this TODO? Are you arguing that startNewSession should be on the store and not on the session module?

I think the answer the the above is yes. However I think there's a lot happening in this method that isn't just store related. Maybe there is a way to keep the session logic in session, and at the end, create a new method about updating the store's session attributes.

I probably should have put this comment before the startNewSession method. My thought is that we're doing a lot of session related activities within the context of a store. Part of me thinks that we should split out the Session related persistence/store attributes into a seaprate Session object, so that the Session Manager can simply manage Sessions. As we've discussed, the store holds a lot of data about the state of the SDK, but a lot of unrelated modules directly modify the state which makes it hard to predict what the current state of the SDK is, so I thought a quick path to tidying things up would be to create a startNewSession method in the store to clean some things up.

rmi22186 · 2024-01-19T22:44:55Z

src/store.ts

+        if (config.hasOwnProperty('usePersistence')) {
+            this.SDKConfig.usePersistence = config.usePersistence;
+        } else {
+            this.SDKConfig.usePersistence = false;


This should be true by default. Is this false because you are in the middle of development?

Good catch. I made this default to false before we made our decision to default it to true. I'll update.

alexs-mparticle · 2024-01-29T19:07:46Z

src/identity.js

+            // TODO: Maybe create an updateIdentity method?
+            // Store relevant identity settings into Store
+            mpInstance._Store.mpid = currentMPID;
+            mpInstance._Store.currentSessionMPIDs = currentSessionMPIDs;


Lines 20 and 21 can just be removed in favor of this approach.

Line 23 can be combined with the check on 18 so it's using Persistence or Store.

Suggested change

mpInstance._Store.currentSessionMPIDs = currentSessionMPIDs;

mpInstance._Store.currentSessionMPIDs = currentSessionMPIDs;

persistence.cu = currentMPID;

persistence.gs.csm = currentSessionMPIDs;

alexs-mparticle · 2024-01-29T19:14:16Z

src/identity.js

+                    mpInstance._Store.setLastSeenTime(previousMPID);
                }

+                // QUESTION: Does this only matter for cookies or do we need it for


It's called cookies, but actually applies to all persistence methods.

Also requires access to the DOM, so we may need a control for that to make sure dom exists.

alexs-mparticle · 2024-01-29T19:32:15Z

src/store.ts

+    userAttributes?: Dictionary<UserAttributes>;
+    getUserAttributes?(mpid: MPID): UserAttributes;
+
+    // QUESTION: Is there a difference between this and currentUserIdentities?


Suggested change

// QUESTION: Is there a difference between this and currentUserIdentities?

// QUESTION: Is there a difference between this and currentUserIdentities?

alexs-mparticle · 2024-01-29T19:35:00Z

src/store.ts

+    this.setDeviceId = (deviceId: string): void => {
+        // TODO: This should update persistence
+        this.deviceId = deviceId;
+        mpInstance._Persistence.update();


Do we still need to call update internally?

alexs-mparticle · 2024-01-29T19:41:27Z

src/store.ts

+    };
+
+    this.setConsentState = (mpid: MPID, consentState: ConsentState) => {
+        // QUESTION: What is the context here?


Comment can be removed as it is not necessary.

sonarqubecloud · 2024-01-30T14:21:34Z

Quality Gate failed

Failed conditions

27.5% Duplication on New Code (required ≤ 3%)
C Reliability Rating on New Code (required ≥ A)

See analysis details on SonarCloud

Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Initial attempt to decouple persistence

09d7944

alexs-mparticle requested a review from rmi22186 January 19, 2024 17:29

rmi22186 reviewed Jan 19, 2024

View reviewed changes

alexs-mparticle added 4 commits January 22, 2024 10:52

Address PR Comments

1cb7f43

Fix typo

2a736a8

Save Point

ea6071a

Save Point

238014f

alexs-mparticle commented Jan 29, 2024

View reviewed changes

alexs-mparticle added 2 commits January 29, 2024 16:12

Fix breaking test

298cc31

Save Point

9915673

rmi22186 force-pushed the development branch from 0853bc2 to dd1920f Compare March 25, 2024 13:56

alexs-mparticle added the backlog label Sep 11, 2024

alexs-mparticle force-pushed the development branch from b6883b6 to b8a1a8f Compare November 21, 2024 21:10

rmi22186 force-pushed the development branch from f03c5ed to 3df8d84 Compare December 11, 2024 17:22

alexs-mparticle force-pushed the development branch from ca2b2d9 to da03fea Compare January 21, 2025 16:00

rmi22186 force-pushed the development branch 5 times, most recently from 905641c to b16bdd2 Compare December 10, 2025 21:07

	persistence?.gs &&
	persistence?.gs.sid &&
	persistence?.gs?.sid &&

	persistence?.gs &&
	persistence?.gs.les &&
	persistence?.gs?.les &&

	// QUESTION: Is there a difference between this and currentUserIdentities?
	// QUESTION: Is there a difference between this and currentUserIdentities?

Initial attempt to decouple persistence #832

Are you sure you want to change the base?

Initial attempt to decouple persistence #832

Uh oh!

Conversation

alexs-mparticle commented Jan 19, 2024

Instructions

Summary

Testing Plan

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

Uh oh!

rmi22186 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexs-mparticle Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexs-mparticle Jan 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jan 30, 2024

Quality Gate failed

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

alexs-mparticle Jan 29, 2024 •

edited

Loading

alexs-mparticle Jan 29, 2024 •

edited

Loading