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

EventsSDK: Support Google Tag Manager #118

Merged
merged 13 commits into from
Nov 15, 2023
Merged

Conversation

ejaffee01
Copy link
Contributor

@ejaffee01 ejaffee01 commented Nov 6, 2023

In order to support Google Tag Manager, we will export a new function just specifically for sending Events API requests from it. This also adds a convertTypesGTM function which is used to convert booleans and numbers represented as strings to their correct types. This is unit tested in convertTypes.test.ts.

J=https://yexttest.atlassian.net/browse/FUS-6055
TEST=auto, unit

Ethan Jaffee added 4 commits October 25, 2023 16:56
In order to support Google Tag Manager, we will export a new function
just specifically for sending Events API requests from it. This also
adds a convertTypesGTM function which is used to convert booleans and
numbers represented as strings to their correct types. This is unit
tested in convertTypes.test.ts.
@ejaffee01 ejaffee01 self-assigned this Nov 6, 2023
src/index.ts Show resolved Hide resolved
@ejaffee01 ejaffee01 marked this pull request as ready for review November 6, 2023 20:11
etc/analytics.api.md Outdated Show resolved Hide resolved
src/convertTypesGTM.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@mtian725 mtian725 left a comment

Choose a reason for hiding this comment

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

  • Link the story

src/index.ts Outdated Show resolved Hide resolved
src/convertTypesGTM.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Show resolved Hide resolved
import { EventPayload } from './EventPayload';

// Define the list of possibly numerical properties to check and convert
const propertiesToCheck = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway to source this automatically from the event payload?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I looked into this a little bit and it appears that since interfaces do not exist at runtime, extracting any information from it at run time is not possible. It seems in order to use typeof on fields of EventPayload the object would need to be initialized.
I also considered just looping through the data object and just determining to convert based on if it is a string representing a boolean or a number, but I am concerned that this will cause more complexity for fields that are strings that look like numbers, but are meant to stay strings in the payload. For example, browserVersion and alike fields.

I know its not ideal to maintain a hard coded list, but I can add this to the wiki for something to remember when adding a new field to the events schema.

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha thanks for the explanation.

Copy link
Contributor

@AjayBenno AjayBenno left a comment

Choose a reason for hiding this comment

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

LGTM, had a small question about the actions warnings but feel free to ship if its expected.

src/index.ts Show resolved Hide resolved

beforeEach(() => {
// Mock window['analyticsEventPayload']
(global as any).window = {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, nothing we can do about these warnings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately not. The alternatives cause actual errors.

  • Changing to unknown leads to Object is of type 'unknown'.

  • Changing to never leads to Property 'window' does not exist on type 'never'.ts(2339)

  • Removing the cast complety leads to `Type '{}' is not assignable to type 'Window & typeof globalThis'.

If we decide this is noisy we can also add an ignore comment to the top of the file in a future PR.
/* eslint-disable @typescript-eslint/no-explicit-any */

@ejaffee01 ejaffee01 merged commit fd43132 into main Nov 15, 2023
12 checks passed
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.

None yet

3 participants