refactor: Migrate Forwarder module to TypeScript #988

alexs-mparticle · 2025-02-28T14:06: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

alexs-mparticle · 2025-02-28T16:14:25Z

src/forwarders.interfaces.ts

+    // constructor?: new () => IMPForwarder;
+    constructor: () => any;
+    name: string;
+    getId: () => number, // Should this be optional?


A side loaded kit may not implement getId so at a minimum, this should be optional.

src/forwarders.interfaces.ts

rmi22186 · 2025-02-28T16:24:36Z

src/forwarders.interfaces.ts

-export type Kit = Dictionary;
+export interface Kit {
+    // constructor?: new () => IMPForwarder;
+    constructor: () => any;


revisit the constructor value?

src/forwarders.interfaces.ts

rmi22186 · 2025-02-28T16:26:17Z

src/forwarders.interfaces.ts

    name: string;
    getId(): number;
+    filters: IKitFilterSettings;


is this only for sideloaded kits?

src/forwarders.interfaces.ts

rmi22186 · 2025-02-28T17:28:33Z

src/forwarders.interfaces.ts

+    screenAttributeFilters: number[];
+
+
+    // Side loaded kit functioanlity in Forwarder methods


Suggested change

// Side loaded kit functioanlity in Forwarder methods

// Side loaded kit functionality in Forwarder methods

rmi22186 · 2025-02-28T17:29:45Z

src/forwarders.interfaces.ts

+
+
+    // Side loaded kit functioanlity in Forwarder methods
+    kitInstance: UnregisteredKit;


i see this as existing on the unregistered Kit, rather than IMPForwarder

rmi22186 · 2025-02-28T17:31:57Z

src/forwarders.interfaces.ts

+    id: number;
+    settings: ForwarderSettings;
+    forwarderStatsUploader: AsyncUploader;
+    isInitialized: boolean;
+    filteringConsentRuleValues: IFilteringConsentRuleValues;
+    filteringUserAttributeValue: IFilteringUserAttributeValue;
+    filteringEventAttributeValue: IFilteringEventAttributeValue;
+    excludeAnonymousUser: boolean;
+    userIdentityFilters: UserIdentityFilters;
+    userAttributeFilters: UserAttributeFilters;


i think a lot of these settings are actually supposed to be on an IKitConfig or IForwarder, and not the IMPForwarder Class. the naming is confusing, so taht's probably why they all got combined into here. maybe this class should be named IMPForwarderClass for now even though I know that's bad practice, but at least so we don't confuse IMPForwarder for the class, which has the class methods on it, vs having properties for an actual forwarder or kit, some of which are found here

this applies to most of the items from line 118-149, but double check

rmi22186 · 2025-02-28T17:32:11Z

src/forwarders.interfaces.ts

+    isSandbox?: boolean;
+    hasSandbox?: boolean;
+    isVisible?: boolean;


these are on the kitconfig and not the IMPForwarder class also

rmi22186 · 2025-03-03T14:47:47Z

src/forwarders.interfaces.ts

+    // constructor?: new () => IMPForwarder;
+    constructor: () => any;
+    name: string;
+    getId: () => number, // Should this be optional?


rmi22186 · 2025-03-03T20:53:32Z

test/jest/sideloadedKit.spec.ts

+    // constructor: class {
+    //     constructor() {}
+    // } as new () => IMPForwarder


flagging to make sure you address this commented code and we don't forget

rmi22186 · 2025-03-03T20:53:46Z

test/jest/utils.ts

+                // @ts-ignore
                constructor: this.constructor,
+                // constructor: this.constructor as { new(): IMPForwarder },


rmi22186 · 2025-03-03T20:54:22Z

test/jest/utils.ts

@@ -74,6 +76,7 @@ export class MockForwarder {
    })
 }

+// TODO: Can we finally get rid of this?


i don't have the historical context here. is this something you did or i did?

I think it's something you introduced but I can't recall why it wasn't ever flagged during code review: #568

rmi22186 · 2025-03-03T20:55:13Z

test/src/tests-forwarders.ts

@@ -3201,6 +3204,8 @@ describe('forwarders', function() {
                    // The received event gets replaced by the last event sent to the forwarder
                    // SideloadedKit11 has received the session start event, but not the Test Event
                    // SideloadedKit22 will receive the Test Event
+
+                    console.log('EventName', window.SideloadedKit11.instance.receivedEvent.EventName);


test/src/tests-forwarders.ts

alexs-mparticle · 2025-03-05T15:15:29Z

src/apiClient.ts

@@ -23,7 +23,7 @@ export interface IAPIClient {
    ) => void;
    initializeForwarderStatsUploader: () => AsyncUploader;
    prepareForwardingStats: (
-        forwarder: MPForwarder,
+        forwarder: IMPForwarder,


Note to self: This might need to be a RegisteredKit

alexs-mparticle · 2025-03-05T15:15:49Z

src/apiClient.ts

@@ -192,7 +192,7 @@ export default function APIClient(
    };

    this.prepareForwardingStats = function(
-        forwarder: MPForwarder,
+        forwarder: IMPForwarder,


This may need to become a RegisteredKit

sonarqubecloud · 2025-03-18T21:01:43Z

Quality Gate passed

Issues
27 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.2% Duplication on New Code

See analysis details on SonarQube Cloud

alexs-mparticle force-pushed the refactor/SQDSDKS-4475-forwarder-typescript branch from 5afc5b6 to 948c6a6 Compare February 28, 2025 14:10

alexs-mparticle changed the base branch from test/SQDSDKS-5767-forwarder-tests-ts to development February 28, 2025 14:10

alexs-mparticle force-pushed the refactor/SQDSDKS-4475-forwarder-typescript branch from 948c6a6 to 9acfdc4 Compare February 28, 2025 14:21

alexs-mparticle commented Feb 28, 2025

View reviewed changes