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

Refactor the SDK to use the new HTTP event tracker #410

Merged
merged 23 commits into from
Aug 23, 2024
Merged

Conversation

marcospassos
Copy link
Member

@marcospassos marcospassos commented Aug 2, 2024

Summary

This PR swaps out the WebSocket implementation with an HTTP-based one for tracking events.

I've also made a fix in their pull request that was uncovered when I changed the implementation.

We have a queue that stores events in the local store in case they cannot be delivered. When the SDK is initialized again, it attempts to deliver the pending events as the first task.

However, there were two issues:

  1. The key used to store the events was based on the tab ID, which meant that events tracked in one tab would not be transmitted by another tab.
  2. Additionally, the PersistedQueue implementation kept a copy of the state in memory to avoid re-parsing the JSON for every read/write operation. However, changing the behavior to have a single source of truth would cause one tab to override the other. Therefore, I refactored it by removing this optimization.

Important

Before merging this PR, we need to make sure of two things:

  • Testing in a real, live environment before releasing
  • Confirming the correct non-retryable status codes

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

Copy link
Member

@renan628 renan628 left a comment

Choose a reason for hiding this comment

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

Amazingly simpler

src/channel/httpBeaconChannel.ts Outdated Show resolved Hide resolved
test/channel/httpBeaconChannel.test.ts Outdated Show resolved Hide resolved
test/container.test.ts Outdated Show resolved Hide resolved
Copy link

pkg-pr-new bot commented Aug 16, 2024

commit: 4dd7c1e

npm i https://pkg.pr.new/@croct/sdk@410

Open in Stackblitz

src/channel/httpBeaconChannel.ts Outdated Show resolved Hide resolved
src/channel/queuedChannel.ts Outdated Show resolved Hide resolved
Copy link

codeclimate bot commented Aug 23, 2024

Code Climate has analyzed commit 4dd7c1e and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 99.8% (-0.1% change).

View more on Code Climate.

@marcospassos marcospassos merged commit d9972b2 into master Aug 23, 2024
7 checks passed
@marcospassos marcospassos deleted the http-refactor branch August 23, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants