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: Refactor request logic #1055

Merged
merged 57 commits into from
Mar 8, 2024
Merged

feat: Refactor request logic #1055

merged 57 commits into from
Mar 8, 2024

Conversation

benjackwhite
Copy link
Collaborator

@benjackwhite benjackwhite commented Mar 5, 2024

Changes

Our code paths around making api requests is messy to say the least. Makes it hard to reason on, hard to make changes and includes a lot of spaghetti code that inflates the bundle size.

  • Simplify everything down to a single request function that handles choosing the right available transport.
  • Refactored a bunch of nested inherited types that meant it was super unclear what was going on
  • Simplified compression greatly - only request needs to know about the internals of it now
  • Makes base64 compression the default for capture calls (this was implicitly the default before - now it is explicit)
  • Make retriable and non-retriable requests explicit
  • Add timeouts

This is a pretty huge PR 😬 - not much to be done about it other than say - sorry! Focused on making sure all tests pass but there is a lot of spaghetti logic before that was unpicked hence the big diff.

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!)

Copy link

github-actions bot commented Mar 5, 2024

Size Change: -13.5 kB (-2%)

Total Size: 849 kB

Filename Size Change
dist/array.full.js 182 kB -3.55 kB (-2%)
dist/array.js 123 kB -3.53 kB (-3%)
dist/es.js 123 kB -3.47 kB (-3%)
dist/exception-autocapture.js 12.1 kB +73 B (+1%)
dist/module.js 123 kB -3.4 kB (-3%)
dist/recorder-v2.js 106 kB +67 B (0%)
dist/recorder.js 58.7 kB +91 B (0%)
dist/surveys-module-previews.js 62.1 kB +87 B (0%)
dist/surveys.js 58.4 kB +87 B (0%)

compressed-size-action

benjackwhite and others added 5 commits March 6, 2024 14:58
@benjackwhite benjackwhite requested a review from pauldambra March 6, 2024 14:21
@@ -65,7 +63,8 @@ export interface AutocaptureConfig {

export interface PostHogConfig {
api_host: string
api_method: string
/** @deprecated - This property is no longer supported */
api_method?: string
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This never made sense as different requests require different methods

@benjackwhite benjackwhite requested a review from neilkakkar March 8, 2024 05:58
Copy link
Contributor

@neilkakkar neilkakkar left a comment

Choose a reason for hiding this comment

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

Ran locally, all seems to work 👍 - great job!

@benjackwhite benjackwhite merged commit fb32c34 into main Mar 8, 2024
13 checks passed
@benjackwhite benjackwhite deleted the feat/request-refactor branch March 8, 2024 12:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump minor Bump minor version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants