Skip to content

fix: add logic to convert traits to strings #763

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

Merged
merged 3 commits into from
Feb 27, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/core/src/info.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
export const libraryInfo = {
name: '@segment/analytics-react-native',
version: '2.12.0',
version: '2.13.0',
};
34 changes: 31 additions & 3 deletions packages/plugins/plugin-firebase/src/FirebasePlugin.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,17 +5,45 @@ import {
ScreenEventType,
TrackEventType,
} from '@segment/analytics-react-native';
import identify from './methods/identify';
import screen from './methods/screen';
import track from './methods/track';
import reset from './methods/reset';
import firebaseAnalytics from '@react-native-firebase/analytics';

export class FirebasePlugin extends DestinationPlugin {
type = PluginType.destination;
key = 'Firebase';

identify(event: IdentifyEventType) {
identify(event);
async identify(event: IdentifyEventType) {
if (event.userId !== undefined) {
await firebaseAnalytics().setUserId(event.userId!);
}
if (event.traits) {
let eventTraits = event.traits;
let safeTraits: Record<string, string>;
Copy link

Choose a reason for hiding this comment

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

why make this a separate declaration? =)


const checkType = (value: unknown) => {
return typeof value === 'object' && !Array.isArray(value);
};
safeTraits = Object.keys(eventTraits).reduce(
(acc: Record<string, string>, trait) => {
if (checkType(eventTraits[trait])) {
this.analytics?.logger.warn(
'We detected an object or array data type. Firebase does not accept nested traits.'
Copy link

@silesky silesky Feb 27, 2023

Choose a reason for hiding this comment

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

@alanjcharles checkType explicitly does not guard against arrays. Should this message be 'We detected a plain object data type'?

Copy link
Contributor

Choose a reason for hiding this comment

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

oh yeah, nice catch!

);

return acc;
}
if (trait !== undefined) {
acc[trait] = eventTraits[trait]!.toString();
}
return acc;
},
{}
);

await firebaseAnalytics().setUserProperties(safeTraits);
}
return event;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import type { IdentifyEventType } from '@segment/analytics-react-native';
import identify from '../identify';
import { FirebasePlugin } from '../../FirebasePlugin';

const mockSetUserId = jest.fn();
const mockSetUserProperties = jest.fn();
Expand All @@ -15,6 +15,7 @@ describe('#identify', () => {
});

it('forwards the identify event with ID only', async () => {
const plugin = new FirebasePlugin();
const event = {
type: 'identify',
userId: '123',
Expand All @@ -23,14 +24,15 @@ describe('#identify', () => {
timestamp: '00000',
} as IdentifyEventType;

await identify(event);
await plugin.identify(event);

expect(mockSetUserId).toHaveBeenCalledTimes(1);
expect(mockSetUserProperties).not.toHaveBeenCalled();
expect(mockSetUserId).toHaveBeenCalledWith('123');
});

it('forwards the identify event with ID and properties', async () => {
const plugin = new FirebasePlugin();
const event = {
type: 'identify',
userId: '123',
Expand All @@ -42,7 +44,7 @@ describe('#identify', () => {
},
} as IdentifyEventType;

await identify(event);
await plugin.identify(event);

expect(mockSetUserId).toHaveBeenCalledTimes(1);
expect(mockSetUserProperties).toHaveBeenCalledTimes(1);
Expand All @@ -51,6 +53,7 @@ describe('#identify', () => {
});

it('forwards the identify event without ID', async () => {
const plugin = new FirebasePlugin();
const event = {
type: 'identify',
anonymousId: 'anon',
Expand All @@ -61,7 +64,7 @@ describe('#identify', () => {
},
} as IdentifyEventType;

await identify(event);
await plugin.identify(event);

expect(mockSetUserId).not.toHaveBeenCalled();
expect(mockSetUserProperties).toHaveBeenCalledTimes(1);
Expand Down
11 changes: 0 additions & 11 deletions packages/plugins/plugin-firebase/src/methods/identify.ts

This file was deleted.