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

feat: add global user properties plugin #577

Merged
merged 15 commits into from
Sep 18, 2023
Merged

Conversation

kelvin-lu
Copy link
Contributor

Summary

This PR adds a simple enrichment plugin that transforms the user properties field into global_user_properties - a beta feature used internally first

Checklist

  • Does your PR title have the correct title format?
  • Does your PR have a breaking change?:

@kelvin-lu kelvin-lu requested review from kevinpagtakhan and a team September 11, 2023 16:44
Copy link

@kelsonpw kelsonpw left a comment

Choose a reason for hiding this comment

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

It seems like there is a new CHANGELOG.md file, but the diff indicates it's empty.

Just a few comments and questions to address, otherwise LGTM.

packages/plugin-global-user-properties/package.json Outdated Show resolved Hide resolved
packages/plugin-global-user-properties/README.md Outdated Show resolved Hide resolved
@kelsonpw kelsonpw requested a review from a team September 11, 2023 18:44
@liuyang1520 liuyang1520 requested a review from a team September 11, 2023 18:58
@qingzhuozhen qingzhuozhen requested a review from a team September 12, 2023 00:13
@kevinpagtakhan
Copy link
Collaborator

@justin-fiedler should we point this pr to v1.x?

@kelvin-lu
Copy link
Contributor Author

kelvin-lu commented Sep 14, 2023

for other events which are not belong to amplitude special events, if there is no user_properties, do we want to set it to null?

I think returning the event unmodified would work in these cases. I realize i meant to type as TrackEvent and not Base, I can fix that

isNotSpecialAmplitudeEvent means no event_type is '$identify', isAmplitudeIdentifyEvent means the event_type = '$identify'.

The type narrowing is not what i'm expecting it to do, but ideally I'm only modifying events of type IdentifyEvent and BaseEvent - really, I'm trying to avoid mutating group identify and revenue events, and other special event types that may come along the way

@kelvin-lu
Copy link
Contributor Author

Thanks everyone for the comments (and sorry for the amount of copy-pasta I left on the ground). I've updated the readme and the tests to include a modules.d.ts that fixes the types.

Unfortunately, ts-jest is having some issues picking up this file, so the tests use a couple of any's. I'll make this a follow up ticket if this isn't a blocker to this pr.

@kelvin-lu
Copy link
Contributor Author

@justin-fiedler @yuhao900914 I've limited my changes to the plugin package - I'll be merging this if there are no concerns

@kelvin-lu kelvin-lu merged commit 0572dcb into main Sep 18, 2023
3 checks passed
@kelvin-lu kelvin-lu deleted the feat-global-user-properties branch September 18, 2023 15:28
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.

7 participants