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 property so we can check if a client is using a proxy #1084

Merged
merged 7 commits into from
Mar 18, 2024

Conversation

zlwaterfield
Copy link
Contributor

@zlwaterfield zlwaterfield commented Mar 14, 2024

Changes

We are trying to determine whether or not a user is sending a events via a proxy in the dashboard so we can customize the experience for them. This will allow us to determine that.

Checklist

  • Tests for new code (see advice on the tests we use)
  • Accounted for the impact of any changes across different browsers
  • Accounted for backwards compatibility of any changes (no breaking changes in posthog-js!)

@zlwaterfield zlwaterfield self-assigned this Mar 14, 2024
Copy link

github-actions bot commented Mar 14, 2024

Size Change: +87.6 kB (+10%) ⚠️

Total Size: 941 kB

Filename Size Change
dist/array.full.js 224 kB +41.4 kB (+23%) 🚨
dist/array.js 124 kB -532 B (0%)
dist/es.js 124 kB -532 B (0%)
dist/module.js 124 kB -532 B (0%)
dist/recorder-v2.js 106 kB +153 B (0%)
dist/recorder.js 106 kB +47.7 kB (+81%) 🆘
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 12.1 kB
dist/surveys-module-previews.js 62 kB
dist/surveys.js 58.3 kB

compressed-size-action

@zlwaterfield
Copy link
Contributor Author

@pauldambra I'd love your thoughts on this PR and implementation.

Another route would be to add an $api_host property and do the work in the PostHog dashbaord to determine if it was a proxy. I wasn't sure if we wanted to be storing that data or not.

src/posthog-core.ts Outdated Show resolved Hide resolved
src/posthog-core.ts Outdated Show resolved Hide resolved
src/posthog-core.ts Outdated Show resolved Hide resolved
src/utils/request-router.ts Outdated Show resolved Hide resolved
@zlwaterfield zlwaterfield changed the title feat: add $using_proxy property feat: add property so we can check if a client is using a proxy Mar 15, 2024
Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Sweet - you will want to add the bump patch label so that when merged it automatically publishes.

You might get tripped up by tests here as a bunch are really badly setup so are probably throwing due to api_host not being set (too much mocking...)

@zlwaterfield
Copy link
Contributor Author

Sweet - you will want to add the bump patch label so that when merged it automatically publishes.

You might get tripped up by tests here as a bunch are really badly setup so are probably throwing due to api_host not being set (too much mocking...)

Sounds good, I'll add a few more tests and test things before planning the patch.

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

Pushed a change to fix the tests as it might not have been super obvious how to fix it.

Just needs a test for this logic now and its good

@zlwaterfield zlwaterfield marked this pull request as ready for review March 18, 2024 14:20
@zlwaterfield
Copy link
Contributor Author

@benjackwhite should this property be added to the posthog in the taxonomy and property definitions?

Copy link
Collaborator

@benjackwhite benjackwhite left a comment

Choose a reason for hiding this comment

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

@zlwaterfield yeah would be good to add to the list but not urgent

@zlwaterfield zlwaterfield added the bump patch Bump patch version when this PR gets merged label Mar 18, 2024
@zlwaterfield zlwaterfield merged commit e0d0801 into main Mar 18, 2024
17 checks passed
@zlwaterfield zlwaterfield deleted the add-using-proxy-property branch March 18, 2024 15:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants