refactor: Migrate Types Module to TypeScript #950

alexs-mparticle · 2024-11-21T18:53:22Z

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

Migrates Types module to TypeScript and refactor to use objects as constants instead of enums.
For our usage, using objects as constants offers us the ability to verify we're passing in some server side values rather than just using the correct enum element.
For example, treating MessageTypes as a number when making conditional comparisons while at the same time verifying that we are using the correct MessageType constant.

Testing Plan

Was this tested locally? If not, explain why.
Run automated tests and verify they behave as expected
Manually test external interfaces for Types to verify they return as expected

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

Closes https://go.mparticle.com/work/SQDSDKS-5218

src/identity.js

jared-dickman · 2024-11-21T21:11:41Z

src/types.ts

+// Dictionary that contains MessageTypes that will
+// trigger an immediate upload.
+export const TriggerUploadType = {
+    [MessageType.Commerce]: 1 as const,


if these are types, should they have the same value 1?

This is something that probably needs to be refactored. Were using the values as a sort of lookup table in the batchUploader when it might be cleaner to do this as a function instead. But that is likely out of scope for this PR.

After thinking about this, I decided we should probably not be doing this as a "type" but rather as a simple lookup inside the actual BatchUploader module.

rmi22186

Looks great so far. Putting this comments in in time for our sync. Still need to finish going through types.ts and utils.ts, and the tests.

src/apiClient.ts

src/batchUploader.ts

rmi22186 · 2024-11-22T14:40:03Z

src/batchUploader.ts

+            MessageType.UserIdentityChange,
+        ] as const;
+
+        return !this.batchingEnabled || priorityEvents.includes(eventDataType as typeof priorityEvents[number]);


Is this some sort of TS black magic? shouldn't number should be defined somewhere, right?

Sort of. number is actually being used as a type and not as a variable. This is saying that eventDataType should be treated as a type of priorityEvent which is an array of numbers.

This will transpile to priorityEvents.includes(eventDataType) but allows TypeScript to enforce that eventDataType is treated as a parameter that is compatible with the members of priorityEvents.

src/identity-user-interfaces.ts

rmi22186 · 2024-11-22T14:52:15Z

src/types.ts

+    UserIdentityChange: 18 as const,
+};
+
+export const EventType = {


Can we add a Jira here to split getName from the enums (perhaps a general jira to do that for all of the types here when we are mixing and matching improperly)? i don't think this is necessary for v3 because i'm envisioning V3 as more of a "light" upgrade where the only breaking changes are deprecated methods, then in V4 we break the APIs in 1 fell swoop.

From my conversations with @einsteinx2, it sounds like we may be making some architectural changes to our event translation logic. I think rather than making a ticket for sustained engineering, we should only address this for when we decide on what is actually necessary for v3+.

src/kitFilterHelper.ts

src/sdkRuntimeModels.ts

src/types.ts

rmi22186 · 2024-11-22T20:54:04Z

src/types.ts

+export const ProfileMessageType = {
+    Logout: 3 as const,
+};


small nit. similar to what you did with trigger uploads - since this is only used in the logout call in identity.js, might be worth moving it over there. or since it's a .js file still, just remembering (or adding a ticket) to move this over there.

My reasoning for moving the Trigger uplooads is because I felt it wasn't really a type but rather a control that is specific to batchUploader. In this case, ProfileMessageType makes more sense (to me) to remain in this file since this file is a collection of our data types (for lack of a better term).

rmi22186 · 2024-11-22T20:55:28Z

src/types.ts

+export const ApplicationTransitionType = {
+    AppInit: 1 as const,
+};


this could be moved to serverModel.ts since it's only called there. and that's already a .ts file unlike the identity.js comment above

Same as above. I think this should stay in here since it's a data type.

test/jest/types.spec.ts

rmi22186 · 2024-11-22T21:06:33Z

test/jest/types.spec.ts

+        it('returns other if the identity type is not found', () => {
+            const { getName } = IdentityType;
+
+            expect(getName(Other)).toBe('Other ID');
+            expect(getName(Other2)).toBe('Other ID');
+            expect(getName(Other3)).toBe('Other ID');
+            expect(getName(Other4)).toBe('Other ID');
+            expect(getName(Other5)).toBe('Other ID');
+            expect(getName(Other6)).toBe('Other ID');
+            expect(getName(Other7)).toBe('Other ID');
+            expect(getName(Other8)).toBe('Other ID');
+            expect(getName(Other9)).toBe('Other ID');
+            expect(getName(Other10)).toBe('Other ID');
+            expect(getName(MobileNumber)).toBe('Other ID');
+            expect(getName(PhoneNumber2)).toBe('Other ID');
+            expect(getName(PhoneNumber3)).toBe('Other ID');
+
+            expect(getName(NaN)).toBe('Other ID');
+        });


I think you uncovered a bug here. all these should probably have an actual Name that's not Other Id. It's likely that the getName switch statement never was updated after these new IdentityTypes were added.

Created https://go.mparticle.com/work/SQDSDKS-6942 to track

rmi22186

tiny last nits, otherwise LGTM

test/jest/types.spec.ts

sonarcloud · 2024-11-25T19:51:07Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud

jared-dickman reviewed Nov 21, 2024

View reviewed changes

src/identity.js Show resolved Hide resolved

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

jared-dickman reviewed Nov 21, 2024

View reviewed changes

alexs-mparticle changed the base branch from development to refactor/ts-migration-blackout-2024 November 21, 2024 21:11

alexs-mparticle marked this pull request as ready for review November 21, 2024 21:48

rmi22186 requested changes Nov 22, 2024

View reviewed changes

alexs-mparticle requested a review from rmi22186 November 22, 2024 16:29

rmi22186 reviewed Nov 22, 2024

View reviewed changes

src/types.ts Outdated Show resolved Hide resolved

rmi22186 reviewed Nov 22, 2024

View reviewed changes

test/jest/types.spec.ts Outdated Show resolved Hide resolved

rmi22186 reviewed Nov 22, 2024

View reviewed changes

test/jest/types.spec.ts Outdated Show resolved Hide resolved

rmi22186 reviewed Nov 22, 2024

View reviewed changes

test/jest/types.spec.ts Outdated Show resolved Hide resolved

rmi22186 requested changes Nov 22, 2024

View reviewed changes

alexs-mparticle requested a review from rmi22186 November 25, 2024 14:46

rmi22186 requested changes Nov 25, 2024

View reviewed changes

test/jest/types.spec.ts Outdated Show resolved Hide resolved

rmi22186 approved these changes Nov 25, 2024

View reviewed changes

refactor: Migrate Types to TypeScript as enums and utils

2cffd75

alexs-mparticle force-pushed the refactor/SQDSDKS-5218-migrate-types branch from 74925d2 to 2cffd75 Compare November 25, 2024 19:50

alexs-mparticle added wait to merge refactor Changes to the structure of the code labels Nov 25, 2024

alexs-mparticle mentioned this pull request Nov 25, 2024

refactor: Migrate Instance Manager Tests to TypeScript #953

Merged

1 task

alexs-mparticle merged commit 5b0113e into refactor/ts-migration-blackout-2024 Nov 27, 2024
31 of 35 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: Migrate Types Module to TypeScript #950

refactor: Migrate Types Module to TypeScript #950

alexs-mparticle commented Nov 21, 2024 •

edited

Loading

jared-dickman Nov 21, 2024

alexs-mparticle Nov 21, 2024

alexs-mparticle Nov 21, 2024

rmi22186 left a comment

rmi22186 Nov 22, 2024

alexs-mparticle Nov 22, 2024

rmi22186 Nov 22, 2024

alexs-mparticle Nov 22, 2024

rmi22186 Nov 22, 2024

alexs-mparticle Nov 25, 2024

rmi22186 Nov 22, 2024

alexs-mparticle Nov 25, 2024

rmi22186 Nov 22, 2024

alexs-mparticle Nov 25, 2024

rmi22186 left a comment

sonarcloud bot commented Nov 25, 2024

refactor: Migrate Types Module to TypeScript #950

refactor: Migrate Types Module to TypeScript #950

Conversation

alexs-mparticle commented Nov 21, 2024 • edited Loading

Instructions

Summary

Testing Plan

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmi22186 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmi22186 left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Nov 25, 2024

Quality Gate passed

alexs-mparticle commented Nov 21, 2024 •

edited

Loading