Skip to content

refactor: convert events module to TS#1258

Open
jaissica12 wants to merge 9 commits into
developmentfrom
refactor/SDKE-1106-migrate-event-modules-to-TS
Open

refactor: convert events module to TS#1258
jaissica12 wants to merge 9 commits into
developmentfrom
refactor/SDKE-1106-migrate-event-modules-to-TS

Conversation

@jaissica12
Copy link
Copy Markdown
Contributor

@jaissica12 jaissica12 commented May 12, 2026

Background

  • As part of the ongoing effort to migrate the mParticle Web SDK from JavaScript to TypeScript, this PR converts the events module and its corresponding test file to TypeScript for improved type safety and developer experience.

What Has Changed

  • Converted src/events.js to src/events.ts with full type annotations on all method parameters, return types, and internal variables
  • Converted test/src/tests-event-logging.js to test/src/tests-event-logging.ts with proper global type declarations and type casts for intentionally invalid test inputs
  • Updated src/events.interfaces.ts to expand the logImpressionEvent signature to accept SDKImpression | SDKImpression[] | SDKProductImpression | SDKProductImpression[]
  • Minor supporting type adjustments in src/sdkRuntimeModels.ts, src/serverModel.ts, and src/store.ts

Screenshots/Video

Screenshot 2026-05-14 at 11 01 50 AM Screenshot 2026-05-14 at 11 02 21 AM Screenshot 2026-05-14 at 11 03 03 AM Screenshot 2026-05-14 at 11 03 45 AM

Checklist

  • I have performed a self-review of my own code.
  • I have made corresponding changes to the documentation.
  • I have added tests that prove my fix is effective or that my feature works.
  • I have tested this locally.

Additional Notes

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

@jaissica12 jaissica12 changed the base branch from master to development May 12, 2026 21:07
@jaissica12 jaissica12 marked this pull request as ready for review May 13, 2026 11:51
@jaissica12 jaissica12 requested a review from a team as a code owner May 13, 2026 11:51
@cursor
Copy link
Copy Markdown

cursor Bot commented May 13, 2026

PR Summary

Medium Risk
Touches core event/commerce logging and location tracking codepaths; changes are largely type-safety oriented but broaden accepted impression/promotion inputs and adjust some runtime handling, so regressions would impact analytics payloads.

Overview
Refactors the Events module to TypeScript with explicit types for event logging, commerce events, DOM event handlers, and location tracking (including safer error logging and globalThis.location navigation).

Expands commerce helper APIs to accept more input shapes: logImpressionEvent now supports both SDKImpression and SDKProductImpression (single or array), and logPromotionEvent accepts single or multiple promotions; related model types and server DTO casting were adjusted to keep payload serialization compatible.

Updates config/runtime typing (SDKConfig.timeout) and converts the event-logging test suite to TypeScript, adding global type declarations and deliberate casts for invalid-input test cases.

Reviewed by Cursor Bugbot for commit d6ac4c2. Bugbot is set up for automated code reviews on this repo. Configure here.

Comment thread src/events.ts
Comment thread src/events.ts Outdated
Comment thread src/events.interfaces.ts
logEvent(event: BaseEvent, eventOptions?: SDKEventOptions): void;
logImpressionEvent(
impression: SDKProductImpression,
impression: SDKImpression | SDKImpression[] | SDKProductImpression | SDKProductImpression[],
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I brought this up with @rmi22186 earlier. I would like for us to not support both arrays and non-arrays for these endpoints, but that might have to be a follow up project.

Copy link
Copy Markdown
Contributor Author

@jaissica12 jaissica12 May 20, 2026

Choose a reason for hiding this comment

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

Created a follow-up ticket to track this: https://go/j/SDKE-1199

Comment thread src/events.ts Outdated
}
} else {
var position = {
this.startTracking = function(callback: ((position?: GeolocationPosition | { coords: { latitude: number | string; longitude: number | string } }) => void) | null): void {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We should define a TrackingCallback type here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

done

Comment thread src/events.ts
};
triggerCallback(callback, position);
} else {
if ('geolocation' in navigator) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If this is a TypeScript migration, why are we adding a conditional here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This conditional was already present in the original events.js, it wasn't added as part of the migration. The only change here was flipping the if/else branch order to address a SonarCloud "unexpected negated condition" warning (the original had if (!mpInstance._Store.isTracking) first).

Comment thread src/events.ts Outdated

function triggerCallback(callback, position) {
function triggerCallback(
callback: ((position?: GeolocationPosition | { coords: { latitude: number | string; longitude: number | string } }) => void) | null,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should be a new type definition.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated

Comment thread src/events.ts Outdated

if (event) {
event.EventCategory = mpInstance._Ecommerce.convertProductActionToEventType(
// TODO(SDKE-1106): Remove `as Function` casts when ecommerce.js is migrated to TS
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// TODO(SDKE-1106): Remove `as Function` casts when ecommerce.js is migrated to TS
// https://go.mparticle.com/work/SDKE-1106

Copy link
Copy Markdown
Contributor Author

@jaissica12 jaissica12 May 20, 2026

Choose a reason for hiding this comment

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

Intentional, this is a temporary cast that will be resolved when the ecommerce module is migrated to TS. Didn't want to create an extra ticket for this so just kept comment and existing migration ticket as refrence

Comment thread src/events.ts Outdated
element,
i;
element: Element,
i: number;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Suggested change
i: number;
elementIndex: number;

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

renamed i to elementIndex

Comment thread src/sdkRuntimeModels.ts

export interface SDKPromotionAction {
PromotionActionType: string;
PromotionActionType: string | valueof<typeof PromotionActionType>;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

can we just use valueof<typeof PromotionActionType> so we can control the values? string would allow any string, thus negating the valueof

Copy link
Copy Markdown
Contributor Author

@jaissica12 jaissica12 May 20, 2026

Choose a reason for hiding this comment

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

Keeping string | valueof for now, tightening to just valueof causes a downstream cast failure in sdkToEventsApiConverter.ts where it converts to PromotionActionActionEnum. Fixing that properly would require changes beyond the scope of this migration PR. We can revisit this in a follow-up.

Comment thread src/store.ts
workspaceToken?: string;
requiredWebviewBridgeName?: string;
isLoggingEnabled?: boolean;
timeout?: number;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

is this a new value introduced in this pr?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

SDKConfig.timeout was already used in the original events.js (for the DOM event handler's setTimeout). It just wasn't declared in the SDKConfig interface, so TypeScript would error on access. Added the type to match the existing runtime shape.

} from './config/constants';
import { IMParticleInstanceManager } from '../../src/sdkRuntimeModels';

declare global {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can't this be imported from elsewhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Added it in constants and importing from there

Comment thread test/src/tests-event-logging.ts Outdated
}
}

const mParticle = window.mParticle as IMParticleInstanceManager;
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

IIRC, this should be imported from config?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated constants.ts to export mParticle with the IMParticleInstanceManager type, so this test file now imports it directly from config. Removed the local cast and the declare global Window declaration.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 3443450. Configure here.

Comment thread src/events.ts
@sonarqubecloud
Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
4.3% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

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.

2 participants