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: handle node frames #28224

Merged
merged 3 commits into from
Feb 6, 2025
Merged

feat: handle node frames #28224

merged 3 commits into from
Feb 6, 2025

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Feb 3, 2025

Problem

Related to PostHog/posthog-js-lite#366

Changes

  • Add new node frame type
  • Funnily enough the context can be added clientside so Node is more similar to Python than it is Javascript
  • I fear that won't be the case if the JS is minified. Wonder if that is ever the case?

Will leave it up to you @oliverb123 as to how we want to structure things. Should there only be a single raw frame type or is that a conflation of types?

How did you test this code?

Copied the message from Kafka to make the test

@daibhin daibhin requested a review from a team February 5, 2025 11:34
Copy link
Contributor

@oliverb123 oliverb123 left a comment

Choose a reason for hiding this comment

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

:shipit:

@oliverb123
Copy link
Contributor

I think it's good to have distinct types for node vs. web javascript frames, even if we do end up doing similar things once we start handling minified frames (as mentioned in the other review). For now, I think we sprint towards what we need to move our own node services over, and bulk out the support later - and what you've got here seems to me like what we want to be doing to get there asap.

@daibhin daibhin merged commit 6e8ede2 into master Feb 6, 2025
84 checks passed
@daibhin daibhin deleted the dn-feat/node-handling branch February 6, 2025 19:27
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