Skip to content

Offline support via Transport wrapper #6403

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

Closed
timfish opened this issue Dec 4, 2022 · 0 comments · Fixed by #6884
Closed

Offline support via Transport wrapper #6403

timfish opened this issue Dec 4, 2022 · 0 comments · Fixed by #6884
Assignees

Comments

@timfish
Copy link
Collaborator

timfish commented Dec 4, 2022

Problem Statement

Currently for browser offline caching support there is the Offline integration which has a number of issues:

  • It does not cater for events that fail to send due to network issues
  • Browser navigator.onLine does not offer a reliable way to determine if the machine is connected to the internet. It only tells you that the machine is connected to a network.
    • For example, on Windows, the Docker virtual network adaptor means that onLine is always true 😭
  • Because it sends delayed events via hub.captureEvent() this results in another pass through the integrations which triggers the Dedupe integration Can't get Offline integration to work #3046 (comment).
    • Multiple passes through integrations and event processors opens up a lot of issues and confusion

Solution Brainstorm

It makes more sense for offline support to act at the transport interface so it can catch send failures. Since the transports have recently been re-written in v7 with a functional approach this has become simpler.

const makeOfflineFetchTransport = makeOfflineTransport(makeFetchTransport);

Additionally allow the backing store to be configurable:

const makeOfflineFetchTransport = makeOfflineTransport(makeFetchTransport, indexedDbStore(MAX_QUEUE_LENGTH));

The issue with wrapping Transport directly is that send doesn't pass through rate limit responses so there is no way to wrap that directly and retry for those failures:

export interface Transport {
  send(request: Envelope): PromiseLike<void>;
  flush(timeout?: number): PromiseLike<boolean>;
}

To get around this current limitation, the Electron SDK offline transport wraps the default TransportRequestExecutor and adds offline persisting. This allows us to queue and retry events that have failed due to rate limit errors.

Is this desired in the browser? Should we be retying rate-limited requests?

We could change send to this without it being a breaking change (#6626):

send(request: Envelope): PromiseLike<void | TransportMakeRequestResponse>;

Then all the existing transports can pass through the response.

Another solution would be to add a new function although this won't currently be able to wrap the existing transports because they don't expose/export their TransportRequestExecutor implementations:

export function createOfflineTransport(
  options: InternalBaseTransportOptions,
  makeRequest: TransportRequestExecutor,
  store: PersistStore = makeIndexedDbStore(DEFAULT_TRANSPORT_BUFFER_SIZE),
): Transport {
  //
}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants