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: Fetch support #898

Merged
merged 17 commits into from
Jan 31, 2024
Merged

feat: Fetch support #898

merged 17 commits into from
Jan 31, 2024

Conversation

benjackwhite
Copy link
Collaborator

@benjackwhite benjackwhite commented Nov 15, 2023

Changes

Fixes #547

Some environments don't support xhr at all anymore like chrome extensions. This PR adds fetch as the default method when available.

This PR makes it possible to set the api_transport to fetch.

It also deprecates support for the on_xhr_error in a slightly breaking way. Decided it is better to not call it than to call it with a broken interface. This should have a low impact (it is anyways edge casey in terms of only getting called when things fail).

TODO

  • Add tests for the fetch implementation (followup)
  • "Flag" the fetch implementation so we can gradually test it for real

Checklist

Copy link

github-actions bot commented Nov 15, 2023

Size Change: +6.18 kB (+1%)

Total Size: 772 kB

Filename Size Change
dist/array.full.js 181 kB +1.29 kB (+1%)
dist/array.js 122 kB +1.28 kB (+1%)
dist/es.js 122 kB +1.28 kB (+1%)
dist/exception-autocapture.js 11.9 kB -16 B (0%)
dist/module.js 123 kB +1.28 kB (+1%)
dist/recorder-v2.js 105 kB +1.07 kB (+1%)
ℹ️ View Unchanged
Filename Size
dist/recorder.js 58.5 kB
dist/surveys.js 48.7 kB

compressed-size-action

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR hasn't seen activity in a week! Should it be merged, closed, or further worked on? If you want to keep it open, post a comment or remove the stale label – otherwise this will be closed in another week.

@posthog-bot
Copy link
Collaborator

This PR was closed due to lack of activity. Feel free to reopen if it's still relevant.

@benjackwhite benjackwhite marked this pull request as ready for review January 31, 2024 11:06
Copy link
Collaborator

@mariusandra mariusandra left a comment

Choose a reason for hiding this comment

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

In general, 👍
I do know that we have people using posthog-js with IE11, who might be affected by this if we (eventually) slim the bundle and remove the old XHR route. Not sure what to do with them

@benjackwhite
Copy link
Collaborator Author

In general, 👍 I do know that we have people using posthog-js with IE11, who might be affected by this if we (eventually) slim the bundle and remove the old XHR route. Not sure what to do with them

I don't think we need to remove it tbh. Main thing is this unblocks certain envs where only fetch is supported which is good enough for now

@benjackwhite benjackwhite added the bump minor Bump minor version when this PR gets merged label Jan 31, 2024
@benjackwhite benjackwhite merged commit 71b373a into main Jan 31, 2024
16 checks passed
@benjackwhite benjackwhite deleted the feat/fetch-support branch January 31, 2024 13:39
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.

lint or test for usages of window that would break service workers
3 participants